Update nmethod caching and clean up unused/duplicate methods #71

ashu-mehra · 2025-06-02T14:10:41Z

This PR updates nmethod caching to be based on caching of CodeBlob. It also cleans up some dead code and duplicate methods.

Progress

Change must not contain extraneous whitespace
Change must be properly reviewed (1 review required, with at least 1 Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/leyden.git pull/71/head:pull/71
$ git checkout pull/71

Update a local copy of the PR:
$ git checkout pull/71
$ git pull https://git.openjdk.org/leyden.git pull/71/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 71

View PR using the GUI difftool:
$ git pr show -t 71

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/leyden/pull/71.diff

Using Webrev

Link to Webrev Comment

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

bridgekeeper · 2025-06-02T14:11:24Z

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

openjdk · 2025-06-02T14:12:17Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

ashu-mehra · 2025-06-02T14:12:30Z

@vnkozlov @adinn please review.

mlbridge · 2025-06-02T14:15:58Z

Webrevs

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

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

vnkozlov · 2025-06-04T00:08:42Z

src/hotspot/share/code/aotCodeCache.hpp

+  uint   _blob_offset; // Start of archived blob in the cache
+  uint   _code_offset; // Start of code for an entry of type "Stub"


Why you need 2 different fields for offset?

I did this only for the clarity - using _blob_offset is for storing Stubs doesn't sound correct, so I added _code_offset for the Stubs. If it is confusing, I can keep _blob_offset and remove the _code_offset field.

I would suggest to use old _code_ofset for now. As we discussed on meeting we will need separate _metadata_offset to move all data describing code so we can map whole section of codes directly into CodeCache.

Or we can compromise: _codeblob_offset, _codeblob_size
Stubs are also using CodeBlob. I don't get why you treat stubs differently.

StubGenerator Stubs used CodeBlob, but in AOT CodeCache we don't store their CodeBlobs as we do for adapters/runtime blobs. We instead only copy the assembly code for each stub.

We don't yet store a blob because we are just archiving a couple of special case stubs. But when we rework multi-stub save and restore for each of the stubgen stub groups (initial, cont, etc) then we should probably store/retrieve the code for all stubs in a group as a complete blob.

n.b. I say probably because we have to think about the case where we have errors loading code for a given stubgen blob or where the stubs we need at runtime include code that was not embedded in the saved blob at assembly time (i.e. because of changes on the command line). In the former case we would have to invalidate the loaded blob and recreate a new blob. In the latter we would have to generate missing code into a code buffer and create a secondary blob after we finish loading+generating.

For now I agree with Vladimir - just use code_offset and code_size.

vnkozlov · 2025-06-04T00:10:48Z

Before I start testing and approve it I think we should wait your fix JDK-8358330 integration (my testing is still running) and then merge from mainline to premain.

adinn · 2025-06-05T09:43:19Z

src/hotspot/share/code/aotCodeCache.hpp

    _loaded       = false;
    _not_entrant  = false;
    _load_fail    = false;
-    _ignore_decompile = true;


This leaves field _ignore_decompile uninitialized. Should you be setting it to false here?

adinn · 2025-06-05T13:05:28Z

src/hotspot/share/code/aotCodeCache.hpp

+  uint   _blob_offset; // Start of archived blob in the cache
+  uint   _code_offset; // Start of code for an entry of type "Stub"


We don't yet store a blob because we are just archiving a couple of special case stubs. But when we rework multi-stub save and restore for each of the stubgen stub groups (initial, cont, etc) then we should probably store/retrieve the code for all stubs in a group as a complete blob.

n.b. I say probably because we have to think about the case where we have errors loading code for a given stubgen blob or where the stubs we need at runtime include code that was not embedded in the saved blob at assembly time (i.e. because of changes on the command line). In the former case we would have to invalidate the loaded blob and recreate a new blob. In the latter we would have to generate missing code into a code buffer and create a secondary blob after we finish loading+generating.

adinn · 2025-06-05T13:05:52Z

src/hotspot/share/code/aotCodeCache.hpp

+  uint   _blob_offset; // Start of archived blob in the cache
+  uint   _code_offset; // Start of code for an entry of type "Stub"


For now I agree with Vladimir - just use code_offset and code_size.

shipilev · 2025-06-05T15:19:02Z

I wonder if both #75 and #76 preclude properly testing this PR. Let's integrate those first too.

vnkozlov · 2025-06-05T15:52:32Z

I wonder if both #75 and #76 preclude properly testing this PR. Let's integrate those first too.

Yes, and merge from mainline with all AOT bugs fixes there. I will wait JDK 25 fork before merge.

vnkozlov · 2025-06-05T16:22:08Z

And we need to fix bootstrap sequence: JDK-8358690 and #75

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

…alled before their destructor Signed-off-by: Ashutosh Mehra <asmehra@redhat.com>

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

ashu-mehra added 4 commits May 29, 2025 09:40

Replace apply_relocations with fix_relocations

b87bf40

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

Refactor nmethod caching

d1c1cf4

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

Fix compile failures

c66a925

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

More cleanup

6bab593

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

openjdk bot added the rfr Pull request is ready for review label Jun 2, 2025

ashu-mehra added 2 commits June 2, 2025 10:33

Remove redundant call to clear()

b9346a6

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

Remove CodeBuffer::clear_strings() as the relevant destructors clear the

646769d

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

vnkozlov reviewed Jun 4, 2025

View reviewed changes

adinn reviewed Jun 5, 2025

View reviewed changes

ashu-mehra added 6 commits June 6, 2025 10:17

Remove AOTCodeEntry::_blob_offset

997ba15

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

Merge branch 'premain' into update-nmethod-caching

d27e24b

Fix undefs

463704b

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

Merge 8358330: AsmRemarks and DbgStrings clear() method may not get c…

288c440

…alled before their destructor Signed-off-by: Ashutosh Mehra <asmehra@redhat.com>

Fix bug in accessing nmethod from Method

e9cafde

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

Consolidate handling of asm remarks and dbg strings

f57cb1d

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

		uint _blob_offset; // Start of archived blob in the cache
		uint _code_offset; // Start of code for an entry of type "Stub"

Update nmethod caching and clean up unused/duplicate methods #71

Are you sure you want to change the base?

Update nmethod caching and clean up unused/duplicate methods #71

Uh oh!

Conversation

ashu-mehra commented Jun 2, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Reviewing

Uh oh!

bridgekeeper bot commented Jun 2, 2025

Uh oh!

openjdk bot commented Jun 2, 2025

Uh oh!

ashu-mehra commented Jun 2, 2025

Uh oh!

mlbridge bot commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vnkozlov commented Jun 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shipilev commented Jun 5, 2025

Uh oh!

vnkozlov commented Jun 5, 2025

Uh oh!

vnkozlov commented Jun 5, 2025

Uh oh!

Uh oh!

ashu-mehra commented Jun 2, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 2, 2025 •

edited

Loading