8354887: Preserve runtime blobs in AOT code cache #25019
Conversation
Signed-off-by: Ashutosh Mehra <asmehra@redhat.com>
Signed-off-by: Ashutosh Mehra <asmehra@redhat.com>
Signed-off-by: Ashutosh Mehra <asmehra@redhat.com>
Signed-off-by: Ashutosh Mehra <asmehra@redhat.com>
Ensure CompressedOops::base and CompressedKlssPointers::base does not change in production run Signed-off-by: Ashutosh Mehra <asmehra@redhat.com>
Signed-off-by: Ashutosh Mehra <asmehra@redhat.com>
Signed-off-by: Ashutosh Mehra <asmehra@redhat.com>
Signed-off-by: Ashutosh Mehra <asmehra@redhat.com>
Signed-off-by: Ashutosh Mehra <asmehra@redhat.com>
|
👋 Welcome back asmehra! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@ashu-mehra The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
ashu-mehra
changed the title
|
/cc hotspot-runtime /label remove hotspot |
|
@ashu-mehra |
|
@ashu-mehra |
|
@ashu-mehra |
src/hotspot/cpu/x86/x86_64.ad
Outdated
| if (AOTCodeCache::is_on_for_dump()) { | ||
| // Created runtime_call_type relocation when caching code | ||
| __ lea(r10, RuntimeAddress((address)$meth$$method)); | ||
| } else { | ||
| __ mov64(r10, (int64_t) $meth$$method); | ||
| } |
There was a problem hiding this comment.
I think we should do it always, not conditionally. On AArch64 it is unconditional - relocation processing know how to do that.
There was a problem hiding this comment.
I will update premain code too later.
src/hotspot/share/asm/codeBuffer.hpp
Outdated
| ~CHeapString() { | ||
| os::free((void*)_string); | ||
| _string = nullptr; | ||
| } |
There was a problem hiding this comment.
Move it into .cpp so you don't need to include os.hpp here.
src/hotspot/share/opto/runtime.cpp
Outdated
| @@ -157,6 +158,7 @@ static bool check_compiled_frame(JavaThread* thread) { | |||
| C2_STUB_TYPEFUNC(name), \ | |||
| C2_STUB_C_FUNC(name), \ | |||
| C2_STUB_NAME(name), \ | |||
| (int)C2_STUB_ID(name), \ | |||
src/hotspot/share/opto/runtime.cpp
Outdated
| @@ -170,6 +172,7 @@ static bool check_compiled_frame(JavaThread* thread) { | |||
| notify_jvmti_vthread_Type, \ | |||
| C2_JVMTI_STUB_C_FUNC(name), \ | |||
| C2_STUB_NAME(name), \ | |||
| (int)C2_STUB_ID(name), \ | |||
| #ifdef COMPILER1 | ||
| #include "c1/c1_Runtime1.hpp" | ||
| #endif |
There was a problem hiding this comment.
Conditional includes are placed at the end of includes.
| if (_compressedKlassBase != CompressedKlassPointers::base()) { | ||
| log_debug(aot, codecache, init)("AOT Code Cache disabled: it was created with CompressedKlassPointers::base() = %p vs current %p", _compressedKlassBase, CompressedKlassPointers::base()); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I would suggest to use relocation for klass's base too but not in these changes. I think bailout is fine here.
| return false; | ||
| } | ||
| log_debug(aot, codecache, stubs)("Writing blob '%s' to AOT Code Cache", name); | ||
| log_info(aot, codecache, stubs)("Writing blob '%s' (id=%u, kind=%s) to AOT Code Cache", name, id, AOTCodeEntry::kind_string(entry_kind)); |
There was a problem hiding this comment.
Please, use log_debug() in final changes.
| return reader.compile_code_blob(name, entry_offset_count, entry_offsets); | ||
| CodeBlob* blob = reader.compile_code_blob(name, entry_offset_count, entry_offsets); | ||
|
|
||
| log_info(aot, codecache, stubs)("Read blob '%s' (id=%u, kind=%s) from AOT Code Cache", name, id, AOTCodeEntry::kind_string(entry_kind)); |
| log_trace(aot, codecache, stubs)("asm remark offset=%d, str='%s'", offset, str); | ||
| uint n = write_bytes(&offset, sizeof(uint)); | ||
| if (n != sizeof(uint)) { | ||
| return false; |
There was a problem hiding this comment.
Consider using id_for_C_string() and record ID instead of coping string. These strings should be recorded in C strings table already.
If id_for_C_string() does not find - assert. We should add add_C_string() in missing place.
There was a problem hiding this comment.
The asm remarks and dbg strings are not currently recorded in C string table.
I tried to add these strings by calling AOTCodeCache::add_C_string() in AsmRemarkCollection::insert() but this results in adding LOTS of strings. So I add the strings to the string table only when writing the asm remarks. This keeps the string count in check.
| log_trace(aot, codecache, stubs)("dbg string=%s", str); | ||
| uint len = (uint)strlen(str) + 1; // including '\0' char | ||
| uint n = write_bytes(str, len); |
|
We need to do something about Compressed Klass base: |
|
Which blob/stubs decompress/compress klass using the base? |
|
May be we should use Relocation for it. |
I too feel bailing out is too restrictive. In my tests I have seen this happening too frequently to ignore.
[0] jdk/src/hotspot/cpu/x86/c1_Runtime1_x86.cpp Line 1131 in 1501a5e jdk/src/hotspot/cpu/x86/macroAssembler_x86.cpp Line 5439 in 1501a5e |
|
@vnkozlov How about updating and adding |
|
Yes, please do that in |
|
Test crashes on linux-x64 with debug VM: |
|
Test |
|
I attached hs_err file to RFE in JBS. |
|
@vnkozlov thanks for testing the patch.
I can recreate this test locally. Looking into it. |
Looks like the code for C2 blobs generated in assembly phase is not correct. Look at the instructions generated after the call to runtime method: Comparing it with the case when AOT code cache is not used: For some reason r12 is used instead of null (0x0). r12 is the CompressedOop::base address. I suspect I missed porting a change from premain. @adinn @vnkozlov any idea what that could be? |
|
Easy to fix. Look for checks |
@ashu-mehra Just to explain what is going on here: This is a performance trick. When compressed oops base is null r12 (aka rheapbase) will have been initialized to zero so it can be used as a zero register. This allows, for example, a move instruction to employ a register operand immediate rather than include a 64 bit zero value in the instruction stream, which results in reduced code size. In this case the two moves are zeroing the current Java thread's frame anchor fields, last Java frame pc and sp, which are only set while the thread is in native. This trick is fine when generated code is run in the same JVM but no use if the code is generated in a VM with zero compressed oops base then reloaded into a JVM where it is no longer null. So, Vladimir's advice is to disable this trick when generating AOT code. |
|
In |
|
@adinn Do you remember why we commented It is still not enabled - how we are not crashing in premain? |
That's a news to me. I thought we were setting |
|
@vnkozlov Ergonomics comes into play. It is set to true in cdsConfig.cpp if CacheDataStore != nullptr. |
|
@adinn in premain it is commented out - I don't see any other place it is set. |
I do ;-) Look at cdsConfig.cpp:468 in method CDSConfig::check_vm_args_consistency() |
|
@adinn I think you are looking on old version of |
|
Ah yes, I was looking at an old version. Apologies for that. |
|
I looked back at the history and found that this line got commented out as part of a merge from mainline (date: 2025-01-15 shaid 4cd4c7c). I'm not at all sure why Ioi did this and have no idea why he added my name to the original FIXME. The very next change he made in the premain branch was to update the FIXME comment, resetting it to his name. I also don't understand why this is not biting us in premain. |
|
There could be testing failures which expect some state of compressed oops. I will test enabling this flag. |
|
We were lucky. I reproduced issue in |
|
Unfortunately we can't enable the flag (that is why Ioi commented it, I think): @ashu-mehra please restore verification check for COOP base you had. instead of patching |
|
I think it is fine not use AOT code when the heap base does not match. Based on my test it will happen only with big heap's size difference between assembly and production runs. |
|
@vnkozlov I wonder if that assert needs modification as well. This assert assumes that if I think the assert should be using With this change I can run the helloworld program with |
|
I also hit failure in So we need the relocation fix from current changes in |
I will try your assert fix. |
|
Several AOT tests failed on linux-x64 with flag enabled: |
|
I moved |
|
I think for these changes we should not use AOT code when the heap base does not match. |
|
Mailing list message from John Rose on hotspot-compiler-dev: On 7 May 2025, at 12:37, Vladimir Kozlov wrote:
Won?t ASLR spoil matches on some platforms? The match must depend on how the VM negotiates memory (I?m not saying we need to address it now, if tests ASLR: https://en.wikipedia.org/wiki/Address_space_layout_randomization |
|
@rose00 We have solution for that: we enforce encoding instructions (UseCompatibleCompressedOops flag) and use relocation info to patch correct COOP base address when loading AOT code. |
@vnkozlov for this PR we are relying on having relocation for COOP base, not on enforcing encoding. And that should be able to handle cases where heap base is different in assembly vs prod. Why do you suggest to not use AOT code when the heap base does not match? |
|
To support that you would need to modify x86_64.ad to exclude r12 from usage as 0 as we discussed. One suggestion I have is to skip using only new runtime blobs you are adding when bases do not match because adapters don't have this issue. |
8350209 introduced the framework for storing code in aot code cache and used it for caching i2c/c2i adapters.
This PR extends the
AOTCodeCacheinfrastructure and stores various runtime blobs (shared blobs, C1 and C2 runtime blobs) in the AOT code cache. It adds a new diagnostic flagAOTStubCachingto enable/disable the caching of these blobs.AOTCodeFlags.javatest is extended to coverAOTStubCaching.Progress
Issue
Contributors
<adinn@openjdk.org>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25019/head:pull/25019$ git checkout pull/25019Update a local copy of the PR:
$ git checkout pull/25019$ git pull https://git.openjdk.org/jdk.git pull/25019/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25019View PR using the GUI difftool:
$ git pr show -t 25019Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25019.diff
Using Webrev
Link to Webrev Comment