Update README.md and default local mmtk location #223
Conversation
so that the instructions are correct when users choose a differnt debug level.
to make mmtk dependency value (even if commented) consistent with the default directory structure in README.md
|
@shamouda Thanks for the PR! I think we need to fix the CI scripts before we can merge this. Some of the CI scripts expect the previous location. I'll make another PR for it. |
| @@ -121,10 +121,10 @@ The output jdk is at `./build/linux-x86_64-normal-server-$DEBUG_LEVEL/jdk`. | |||
| **Note:** The above `make` command will build what is known as the [`default` target or "exploded image"](https://github.com/openjdk/jdk11u/blob/master/doc/building.md#Running-make). This build is exclusively meant for developers who want quick and incremental builds to test changes. If you are planning on evaluating your build (be it performance, minimum heap, etc.), then it is *highly advised* to use the `images` target. The `default` target is the (roughly) minimal set of outputs required to run the built JDK and is not guaranteed to run all benchmarks. It may have bloated minimum heap values as well. The `images` target can be built like so: | |||
|
|
|||
| ```console | |||
| $ make CONF=linux-x86_64-normal-server-release THIRD_PARTY_HEAP=$PWD/../mmtk-openjdk/openjdk images | |||
There was a problem hiding this comment.
Could you add a line saying "For performance evaluation, we expect you are using the release build"? That was the original reason the make command was not using $DEBUG_LEVEL and directly using release. I agree that it should have been sign posted a bit better.
There was a problem hiding this comment.
Or better yet, mention it in the note directly above the make command.
There was a problem hiding this comment.
Does the same apply to the "Profile-Guided Optimized Build" section?
If that is the case, then I should probably either:
- undo all the changes (i.e. restore the hardcoded -release values) but add a clarifying note, or
- add "DEBUG_LEVEL=release" before the
makecommands to make it more explicit.
What do you think?
There was a problem hiding this comment.
I think the first one is probably better. And yes, the PGO build is also generally used for evaluation only. The images (and PGO build) have extra steps involved in them which only make sense when you want to perform some kind of evaluation. In the images case, the default build (an unoptimized dev build) is used to bootstrap a production build and in the PGO case, the Rust compiler uses execution profiles to inform inlining decisions.
…images target As per k-sareen's review, users are expected to use the release mode for performance evaluations. So using a hardcoded debug mode here is OK. Updated the note text to clarify that.
|
@qinsoon Which CI script is even using the hardcoded value? I don't see it being used by the old CI script here? Clearly it's passing the tests above. |
We can update the mmtk-core CI to parse and modify the toml semantically. In that case, it doesn't matter which value we put in the toml file. See mmtk/mmtk-core#856 |
|
Ah right it's in the mmtk-core CI script. I see. Yes we should parse it in a more robust way imo. I'll investigate how best to do it tonight. |
What Kunshan posted is about reading data from toml. So we can use |
Yes that makes sense. How about using a slightly non-standard tool like this: https://github.com/TomWright/dasel. I found it by doing a bit of quick googling. |
|
I think when we modify, we should directly modify the TOML file, not the generated JSON from There are TOML libraries for Python, too. Alternatively, there is a tool named |
|
That's why I suggested the |
Minor edits to README