Skip to content
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

Save/load i2c2i adapters #25

Open
wants to merge 24 commits into
base: premain
Choose a base branch
from

Conversation

ashu-mehra
Copy link
Collaborator

@ashu-mehra ashu-mehra commented Oct 23, 2024

This is an attempt to save and load i2c2i adapters along with the adapter handler table.
There are mainly two parts to this change:

  1. Storing of adapter code in the SCCache or AOT code cache.
  2. Storing of adapter handler table in the AOT cache.

Adapter handler table is a map from AdapterFingerPrint to AdapterHnadlerEntry. To store them in AOT cache, AdapterFingerPrint and AdapterHandlerEntry are updated to MetaspaceObj. Both these entities are discovered and added to the cache while processing the Method. When storing the adapter handler table, only the entries that have already been archived are considered. This allows pruning of AdapterHnadlerEntry that may be only reachable through a Method that is not eligible to be archived.

An AdapterHandlerEntry has pointer to the adapter code. Because the AdapterHandlerEntry and the adapter code are stored in separate archives, this link between the AdapterHandlerEntry and the adapter code needs to be removed (see AdapterHandlerEntry::remove_unshareable_info()).
During the production run, as the methods in the AOT cache are adopted, the AdapterHandlerEntry is linked back to the adapter code (see AdapterHandlerEntry::restore_unshareable_info).

All this code is guarded by -XX:[+-]ArchiveAdapters option which defaults to false, but is set to true in CDSConfig during the assembly phase.

Other changes worth mentioning:

  1. Changes to the SCCache infrastructure to make it possible to store and load adapter code. (Thanks to @adinn)
  2. Updating AdapterFingerPrint hashing algorithm to avoid collisions. If there is any collision, then it will prevent finding the adapter code in the SCCache. (Again courtesy of @adinn)

Thanks to @adinn for providing many of these changes.

Performance:
-Xlog:init shows time taken for linking of Methods and making adapters. An example output is:

ClassLoader:
  clinit:                             150us / 4612 events
  link methods:                     28980us / 176893 events
  method adapters:                  15378us / 697 events

Save/load of adapters seem to have improved these stats.

Quarkus -ArchiveAdapters +ArchiveAdapters
link methods 12214us / 58913 events 2700us / 58913 events
method adapters 7793us / 607 events 4402us / 38 events
Spring-petclinic -ArchiveAdapters +ArchiveAdapters
link methods 28980us / 176893 events 7485us / 176893 events
method adapters 15378us / 697 events 7050us / 13 events

However, testing with Quarkus app, I don't see any noticeable improvement in the startup time.


Progress

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

Warnings

 ⚠️ Patch contains a binary file (test/jdk/java/security/ProtectionDomain/AllPerm.jar)
 ⚠️ Patch contains a binary file (test/jdk/sun/security/provider/PolicyFile/TokenStore.keystore)
 ⚠️ Patch contains a binary file (test/jdk/sun/security/provider/PolicyFile/TrustedCert.keystore)
 ⚠️ Patch contains a binary file (test/jdk/sun/security/provider/PolicyFile/TrustedCert.keystore1)
 ⚠️ Patch contains a binary file (test/jdk/sun/security/provider/PolicyParser/ExtDirsA/a.jar)
 ⚠️ Patch contains a binary file (test/jdk/sun/security/provider/PolicyParser/ExtDirsB/b.jar)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25

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

Using diff file

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

Using Webrev

Link to Webrev Comment

Signed-off-by: Ashutosh Mehra <[email protected]>
Signed-off-by: Ashutosh Mehra <[email protected]>
@bridgekeeper
Copy link

bridgekeeper bot commented Oct 23, 2024

👋 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
Copy link

openjdk bot commented Oct 23, 2024

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

@openjdk openjdk bot added the rfr Pull request is ready for review label Oct 24, 2024
@mlbridge
Copy link

mlbridge bot commented Oct 24, 2024

Webrevs

Copy link
Collaborator

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

Very nice changes. Few comments.

src/hotspot/share/code/SCCache.cpp Outdated Show resolved Hide resolved
src/hotspot/share/code/SCCache.cpp Outdated Show resolved Hide resolved
src/hotspot/share/code/SCCache.cpp Outdated Show resolved Hide resolved
Comment on lines 1269 to 1271
// TODO: how to identify code cache full situation now that the adapter() can be
// non-null if AOT cache is in use
#if 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you check next (opposite to check in the following assert)?:

 if (adapter() != nullptr && !adapter()->is_linked()) {

The assumption is that we have enough CodeCache when we loading adapters from APT cache. Otherwise we should bailout (did you test such case?).

Is is_linked() is specific for adapters from AOT cache?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

is_linked() is being set for every AdapterHandlerEntry when its code is either generated or loaded from AOT cache.

Regarding the original block of code that this check pertains to:

  // If the code cache is full, we may reenter this function for the
  // leftover methods that weren't linked.
  if (adapter() != nullptr) {
    return;
  }

The comment seem to indicate that we may reenter this function for a Method* for which adapter code has already been generated. However I am not able to trace the code path that may result in re-entering this function. Can you please explain under what conditions is this possible? @vnkozlov

Copy link
Collaborator

Choose a reason for hiding this comment

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

The comment was added for JDK-7033141
https://hg.openjdk.org/jdk9/jdk9/hotspot/rev/d3b9f2be46ab

I think the comment is incorrect. It should talk about PermGen space based on bug's evaluation:

"If the VM runs out of permgen space while allocating the constant pool cache, it tries to reverify the bytecodes in the methods for the class. But the bytecodes have been rewritten. I'm working on a fix that un-rewrites the bytecodes so that the VM can try again to link this class. I am debugging this now - actually I'm debugging my code that forces the error condition (for testing) since this but only reproduces for a specific error condition.
It's not very unlikely for an application to run out of permgen (or code cache as in bug 6947901) so it is probably worth fixing for jdk 7. The fix is relatively low risk once it's debugged."

JDK-6947901 shows failure with -Xint too.

But I imaging that full CodeCache may also cause failure to create adapters which will cause "un-rewrites" bytecode.

We don't have PermGen anymore. The only issue is space in CodeCache for adapters. Which you can check before loading adapters since you know size of adapters code in AOT cache.

I don't think we currently check that CodeCache size is the same during product run as during AOT Assembly phase. Adapters are allocated in NonNMethod section.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@vnkozlov I added a change to fix this by checking if adapter is shared or not. If it is not shared and is not null, we return, else we continue. This should restore the behavior of returning early if link_method() gets called again due to code cache full.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good!

@openjdk
Copy link

openjdk bot commented Nov 12, 2024

@ashu-mehra this pull request can not be integrated into premain due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout premain-save-i2c2i-v3
git fetch https://git.openjdk.org/leyden.git premain
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge premain"
git push

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Nov 12, 2024
@openjdk openjdk bot removed the rfr Pull request is ready for review label Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-conflict Pull request has merge conflict with target branch
Development

Successfully merging this pull request may close these issues.

3 participants