Skip to content

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

Closed

Conversation

ashu-mehra
Copy link
Contributor

@ashu-mehra ashu-mehra commented May 3, 2025

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 flag AOTStubCaching to enable/disable the caching of these blobs.
AOTCodeFlags.java test is extended to cover AOTStubCaching.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8354887: Preserve runtime blobs in AOT code cache (Enhancement - P4)

Reviewers

Contributors

  • Andrew Dinn <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

ashu-mehra added 9 commits May 3, 2025 00:08
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>
@bridgekeeper
Copy link

bridgekeeper bot commented May 3, 2025

👋 Welcome back asmehra! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented May 3, 2025

@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:

8354887: Preserve runtime blobs in AOT code cache

Co-authored-by: Andrew Dinn <adinn@openjdk.org>
Reviewed-by: kvn, adinn

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 master branch:

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 master branch, type /integrate in a new comment.

@openjdk
Copy link

openjdk bot commented May 3, 2025

@ashu-mehra The following label will be automatically applied to this pull request:

  • hotspot

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.

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label May 3, 2025
@ashu-mehra ashu-mehra changed the title Preserve runtime blobs master 8354887: Preserve runtime blobs in AOT code cache May 3, 2025
@openjdk openjdk bot added the rfr Pull request is ready for review label May 3, 2025
@ashu-mehra
Copy link
Contributor Author

/cc hotspot-runtime
/cc hotspot-compiler

/label remove hotspot

@openjdk openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label May 3, 2025
@openjdk
Copy link

openjdk bot commented May 3, 2025

@ashu-mehra
The hotspot-runtime label was successfully added.

@openjdk openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label May 3, 2025
@openjdk
Copy link

openjdk bot commented May 3, 2025

@ashu-mehra
The hotspot-compiler label was successfully added.

@openjdk openjdk bot removed the hotspot hotspot-dev@openjdk.org label May 3, 2025
@openjdk
Copy link

openjdk bot commented May 3, 2025

@ashu-mehra
The hotspot label was successfully removed.

@mlbridge
Copy link

mlbridge bot commented May 3, 2025

Webrevs

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few comments.

Comment on lines 1863 to 1868
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);
}
Copy link
Contributor

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.

Copy link
Contributor

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.

Comment on lines 300 to 303
~CHeapString() {
os::free((void*)_string);
_string = nullptr;
}
Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constructor too.

@@ -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), \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, align \

@@ -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), \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too.

Comment on lines 25 to 27
#ifdef COMPILER1
#include "c1/c1_Runtime1.hpp"
#endif
Copy link
Contributor

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.

Comment on lines 419 to 422
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;
}
Copy link
Contributor

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));
Copy link
Contributor

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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use log_debug()

Comment on lines 1156 to 1158
log_trace(aot, codecache, stubs)("dbg string=%s", str);
uint len = (uint)strlen(str) + 1; // including '\0' char
uint n = write_bytes(str, len);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

@vnkozlov
Copy link
Contributor

vnkozlov commented May 3, 2025

We need to do something about Compressed Klass base:

java -XX:AOTCache=app.aotcache -XX:+UnlockDiagnosticVMOptions -XX:+AOTAdapterCaching -XX:+AOTStubCaching -Xlog:aot+codecache+init=debug -cp hello.jar HelloWorld
[0.009s][debug][aot,codecache,init] Mapped 548864 bytes at address 0x00007f17b1611000 at AOT Code Cache
[0.009s][info ][aot,codecache,init] Loaded 384 AOT code entries from AOT Code Cache
[0.009s][debug][aot,codecache,init]   Adapters:  total=316
[0.009s][debug][aot,codecache,init]   Shared Blobs: total=14
[0.009s][debug][aot,codecache,init]   C1 Blobs: total=34
[0.009s][debug][aot,codecache,init]   C2 Blobs: total=20
[0.009s][debug][aot,codecache,init]   AOT code cache size: 543392 bytes
[0.009s][debug][aot,codecache,init]   Loaded 1 C strings of total length 28 at offset 521860 from AOT Code Cache
[0.010s][debug][aot,codecache,init] AOT Code Cache disabled: it was created with CompressedKlassPointers::base() = 0x7f56c6000000 vs current 0x7f1762000000
[0.010s][info ][aot,codecache,init] Unable to use AOT Code Cache.
Hellow World!

@vnkozlov
Copy link
Contributor

vnkozlov commented May 3, 2025

Which blob/stubs decompress/compress klass using the base?

@vnkozlov
Copy link
Contributor

vnkozlov commented May 3, 2025

May be we should use Relocation for it.

@ashu-mehra
Copy link
Contributor Author

We need to do something about Compressed Klass base:

I too feel bailing out is too restrictive. In my tests I have seen this happening too frequently to ignore.

Which blob/stubs decompress/compress klass using the base?

CompressedKlassPointers::base() is called by C1 blob for is_instance_of [0].

[0]

__ load_klass(temp2, obj, /*tmp*/temp1);

mov64(tmp, (int64_t)CompressedKlassPointers::base());

@ashu-mehra
Copy link
Contributor Author

@vnkozlov How about updating decode_klass_not_null like this?

  if (CompressedKlassPointers::base() != nullptr) {
    if (AOTCodeCache::is_on_for_dump()) {
      movptr(tmp, ExternalAddress(CompressedKlassPointers::base_addr()));
    } else {
      mov64(tmp, (int64_t)CompressedKlassPointers::base());
    }
    addq(r, tmp);
  }

and adding CompressedKlassPointers::base_addr() to the AOTAddressTable.

@vnkozlov
Copy link
Contributor

vnkozlov commented May 5, 2025

Yes, please do that in decode_klass_not_null().
We update other methods for nmethod caching in JDK 26.

Signed-off-by: Ashutosh Mehra <asmehra@redhat.com>
configuration

Signed-off-by: Ashutosh Mehra <asmehra@redhat.com>
@openjdk openjdk bot removed the rfr Pull request is ready for review label May 12, 2025
Signed-off-by: Ashutosh Mehra <asmehra@redhat.com>
Signed-off-by: Ashutosh Mehra <asmehra@redhat.com>
@openjdk openjdk bot added the rfr Pull request is ready for review label May 12, 2025
@@ -165,6 +166,7 @@ class AOTCodeCache : public CHeapObj<mtCode> {
protected:
class Config {
uint _compressedOopShift;
address _compressedOopBase;
Copy link
Contributor

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 {
Copy link
Contributor

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.

Comment on lines 370 to 374
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);
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@vnkozlov
Copy link
Contributor

@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>
@ashu-mehra
Copy link
Contributor Author

@vnkozlov addressed your comments.
I also noticed the newly added test AOTCodeCompressedOopsTest was consistently failing on macos-aarch64 because for some reason the CompressedOops::shift=3 even for heap size as low as for Xmx128m. So I have updated the test to make it more resilient by reading the 'actual' base and shift from Xlog:cds output.

Copy link
Contributor

@vnkozlov vnkozlov left a 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.

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My testing passed.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label May 14, 2025
@ashu-mehra
Copy link
Contributor Author

@adinn can you please review this as well?

@ashu-mehra
Copy link
Contributor Author

@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);
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Copy link
Contributor

@adinn adinn left a 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.

@ashu-mehra
Copy link
Contributor Author

@adinn thanks for the review

@ashu-mehra
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented May 15, 2025

Going to push as commit c59debb.
Since your change was applied there have been 51 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label May 15, 2025
@openjdk openjdk bot closed this May 15, 2025
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 15, 2025
@openjdk
Copy link

openjdk bot commented May 15, 2025

@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.

@dholmes-ora
Copy link
Member

This change has broken the Zero build - https://bugs.openjdk.org/browse/JDK-8357084

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler hotspot-compiler-dev@openjdk.org hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

4 participants