-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8354887: Preserve runtime blobs in AOT code cache #25019
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 |
@ashu-mehra 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 51 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
@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. |
/cc hotspot-runtime /label remove hotspot |
@ashu-mehra |
@ashu-mehra |
@ashu-mehra |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move it into .cpp
so you don't need to include os.hpp
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constructor too.
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), \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, align \
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), \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too.
#ifdef COMPILER1 | ||
#include "c1/c1_Runtime1.hpp" | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use log_debug()
log_trace(aot, codecache, stubs)("dbg string=%s", str); | ||
uint len = (uint)strlen(str) + 1; // including '\0' char | ||
uint n = write_bytes(str, len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
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 |
Signed-off-by: Ashutosh Mehra <asmehra@redhat.com>
configuration Signed-off-by: Ashutosh Mehra <asmehra@redhat.com>
Signed-off-by: Ashutosh Mehra <asmehra@redhat.com>
Signed-off-by: Ashutosh Mehra <asmehra@redhat.com>
@@ -165,6 +166,7 @@ class AOTCodeCache : public CHeapObj<mtCode> { | |||
protected: | |||
class Config { | |||
uint _compressedOopShift; | |||
address _compressedOopBase; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put it fist to avoid gaps between fields.
import jdk.test.lib.cds.CDSAppTester; | ||
import jdk.test.lib.process.OutputAnalyzer; | ||
|
||
public class AOTCodeCompressedOopsTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ioi also suggested test example which I attached to RFE in JBS.
Looks like your test is very similar and more.
static bool is_dumping_adapters() NOT_CDS_RETURN_(false); | ||
static bool is_using_adapters() NOT_CDS_RETURN_(false); | ||
|
||
static bool is_dumping_stubs() NOT_CDS_RETURN_(false); | ||
static bool is_using_stubs() NOT_CDS_RETURN_(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have singular naming (is_dumping_stub()
) for these methods in premain
branch.
I was debating to do separate RFE for renaming in mainline or may be you can do it here.
It is up to you.
I did not pay attention to these when I work on adapter caching. But now I have to merge from mainline to premain and I noticed difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will make this change.
@ashu-mehra, this looks good with few comments. After you address them, please merge latest jdk - I pushed small related change to limit platforms to run with AOT. After that I will submit new testing. |
Signed-off-by: Ashutosh Mehra <asmehra@redhat.com>
Signed-off-by: Ashutosh Mehra <asmehra@redhat.com>
@vnkozlov addressed your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I submitted testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My testing passed.
@adinn can you please review this as well? |
@vnkozlov thank you for reviewing and testing it. |
@@ -2184,6 +2185,12 @@ void SharedRuntime::generate_deopt_blob() { | |||
} | |||
#endif | |||
const char* name = SharedRuntime::stub_name(SharedStubId::deopt_id); | |||
CodeBlob* blob = AOTCodeCache::load_code_blob(AOTCodeEntry::SharedBlob, (uint)SharedStubId::deopt_id, name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For most shared blobs we have a single entry at offset 0. However, for the deopt blob we have 3 (or 5) extra entry points which are embedded in the deopt blob as field values. Saving and restoring the blob internal state removes the need to pass a count and array of entry offsets into load_code_blob and store_code_blob at this point.
That makes we wonder if we need to do the same with AdapterBlob. If we embedded the offsets that are currently stored in AdapterHandlerEntry into AdapterBlob then we could also avoid having to explicitly pass the count and array of offsets at AOT load/store for adapters. The getters in AdapterHandlerEntry could fetch them indirectly or else the entry could cache them locally when it is initialized depending on whether we care about a memory indirection. Maybe this would make things more uniform?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, I agree and I have the similar idea of storing entry points in AdapterBlob just like DeoptimizationBlob. Currently the pointer to AdapterBlob is not maintained anywhere. So the AdapterHandlerEntry would also have to maintain pointer to AdapterBlob.
I was actually wondering if we can eliminate AdapterHandlerEntry by also storing AdapterFingerprint in the AdapterBlob. The Method can then have a pointer to AdapterBlob.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will open an RFE to move entry points to AdapterBlob.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, let's do that in follow up RFE.
@adinn can you approve current changes so we can proceed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked through everything and could not spot any issues other than moving the adapter offsets into the blob. If we do decide to change that it can wait until we fold in the StubGen stub save/restore -- where we will need to save the blob plus extra info about the stubs within the blob and offsets to oner more entries within each stub.
We will also eventually need to clean up registration of stub entry addresses in the address table but that has to wait until I have completed implementing generator code to produce an enum that identifies all generated entries.
So I think this is ok to go in for now.
@adinn thanks for the review |
/integrate |
Going to push as commit c59debb.
Your commit was automatically rebased without conflicts. |
@ashu-mehra Pushed as commit c59debb. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
This change has broken the Zero build - https://bugs.openjdk.org/browse/JDK-8357084 |
8350209 introduced the framework for storing code in aot code cache and used it for caching i2c/c2i adapters.
This PR extends the
AOTCodeCache
infrastructure and stores various runtime blobs (shared blobs, C1 and C2 runtime blobs) in the AOT code cache. It adds a new diagnostic flagAOTStubCaching
to enable/disable the caching of these blobs.AOTCodeFlags.java
test is extended to coverAOTStubCaching
.Progress
Issue
Reviewers
Contributors
<adinn@openjdk.org>
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25019/head:pull/25019
$ git checkout pull/25019
Update a local copy of the PR:
$ git checkout pull/25019
$ git pull https://git.openjdk.org/jdk.git pull/25019/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25019
View PR using the GUI difftool:
$ git pr show -t 25019
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25019.diff
Using Webrev
Link to Webrev Comment