8347830: [premain] UseCompatibleCompressedOops is broken after merging with mainline #67
Conversation
|
@ashu-mehra and @adinn please look. |
|
👋 Welcome back kvn! A progress list of the required criteria for merging this PR into |
|
@vnkozlov This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
Webrevs
|
| if (UseCompressedOops && AOTCodeCache::is_caching_enabled()) { | ||
| FLAG_SET_ERGO_IF_DEFAULT(UseCompatibleCompressedOops, true); | ||
| } else if (!FLAG_IS_DEFAULT(UseCompatibleCompressedOops)) { | ||
| FLAG_SET_ERGO(UseCompatibleCompressedOops, false); |
There was a problem hiding this comment.
Why do we need to set it to false explicitly here? If it is not default shouldn't we use whatever value the user specified?
There was a problem hiding this comment.
I consider the flag make sense only for AOT code and not for general usage. See description: globals.hpp#L132
We don't want generate more complex encoding if we don't generate/use AOT code. Or compressed oops are off.
We had also recently discussed how we should disable some AOT features if AOT caching is off - the conclusion was that we should silently ignore them (may be produce UL output on demand). That is why I used ERGO setting.
|
|
||
| bool unscaled = false; | ||
| bool zerobased = false; | ||
| bool heapbased = UseCompatibleCompressedOops; |
There was a problem hiding this comment.
Use of heapbased is a bit confusing to me. Can we restructure it like this:
bool unscaled = false;
bool zerobased = false;
if (UseCompatibleCompressedOops) {
unscaled = (heap_end_address <= (char*)UnscaledOopHeapMax);
zerobased = (heap_end_address <= (char*)OopEncodingHeapMax);
}
size_t noaccess_prefix = (UseCompatibleCompressedOops || !zerobased) ? noaccess_prefix_size : 0;
There was a problem hiding this comment.
I can do that. I got names from CompressedOops::Mode values
|
@ashu-mehra I refactored code as you asked. |
|
@ashu-mehra I refactored code as you asked. I looked on our old code where we added this flag and found that I missed 2 other places to check which I added now. |
|
Thank you. @ashu-mehra |
The issue was that we did not allocate space "noaccess prefix" before heap for read-only page when we setup type of encoding with
UseCompatibleCompressedOopsflag. I update code which reserves heap for compressed oops to take into account this flag.An other issue was that
UseCompatibleCompressedOopsflag was set during CDS arguments consistency checkup. ButUseCompressedOopsflag is set by GC ergonomics. I moved code to CDS ergonomics setting method.During testing I hit assert that AOT code address table is missing some code blob address. But it was actually card table base address which pointed inside CodeCache. I added special case to check for it early.
Tested premain-tier1.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/leyden.git pull/67/head:pull/67$ git checkout pull/67Update a local copy of the PR:
$ git checkout pull/67$ git pull https://git.openjdk.org/leyden.git pull/67/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 67View PR using the GUI difftool:
$ git pr show -t 67Using diff file
Download this PR as a diff file:
https://git.openjdk.org/leyden/pull/67.diff
Using Webrev
Link to Webrev Comment