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

8262376: ReplaceCriticalClassesForSubgraphs.java fails if --with-build-jdk is used #3780

Conversation

iklam
Copy link
Member

@iklam iklam commented Apr 29, 2021

When CDS tries to load sun.util.resources.cldr.provider.CLDRLocaleDataMetaInfo from the classlist, the VM has already finished the "early" bootup phase. To load this class, the module system tries to create an instance of jdk.internal.module.SystemModuleFinders$SystemModuleReader and store that into the ArchivedBootLayer. However, the SystemModuleReader class is also not yet loaded, so the VM loads it on demand. As a result, SystemModuleReader is treated as a "non-early class".

The problem is now ArchivedBootLayer refers to a non-early class. At runtime, if JVMTI ClassLoadHook is enabled, HeapShared will refuse to load ArchivedBootLayer from the archived heap. The reason is explained by this in heapShared.cpp:

// Note: if a ArchivedKlassSubGraphInfoRecord contains non-early classes, and JVMTI
// ClassFileLoadHook is enabled, it's possible for this class to be dynamically replaced. In
// this case, we will not load the ArchivedKlassSubGraphInfoRecord and will clear its roots.

However, this doesn't make sense for ArchivedBootLayer -- it's loaded in the early phase at runtime, so any classes referenced by ArchivedBootLayer will also be loaded in the early phase. This means JVMTI ClassLoadHook cannot replace SystemModuleReader.

Hence, the fix is simple -- for any classes referenced by the 3 "full-module-graph" subgraphs, we always treat them as "early" classes, regardless of when they are loaded during dump time.

// Entry fields for subgraphs archived in the open archive heap region (full module graph).
static ArchivableStaticFieldInfo fmg_open_archive_subgraph_entry_fields[] = {
  {"jdk/internal/loader/ArchivedClassLoaders", "archivedClassLoaders"},
  {"jdk/internal/module/ArchivedBootLayer", "archivedBootLayer"},
  {"java/lang/Module$ArchivedData", "archivedData"},
};

The validate the fix, I modified the test case to always load CLDRLocaleDataMetaInfo during dump time. The test failed without the fix and passed with the fix.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8262376: ReplaceCriticalClassesForSubgraphs.java fails if --with-build-jdk is used

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3780/head:pull/3780
$ git checkout pull/3780

Update a local copy of the PR:
$ git checkout pull/3780
$ git pull https://git.openjdk.java.net/jdk pull/3780/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3780

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3780.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 29, 2021

👋 Welcome back iklam! 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 Apr 29, 2021

@iklam The following label will be automatically applied to this pull request:

  • hotspot-runtime

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-runtime hotspot-runtime-dev@openjdk.org label Apr 29, 2021
@iklam iklam marked this pull request as ready for review April 29, 2021 00:43
@openjdk openjdk bot added the rfr Pull request is ready for review label Apr 29, 2021
@mlbridge
Copy link

mlbridge bot commented Apr 29, 2021

Webrevs

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

Hi Ioi,

Based on your detailed description the fix seems quite reasonable.

Does this mean that when using CDS some classes are not subject to the application of the JVM TI ClassFileLoadHook? And if so, is that constraint documented somewhere?

Thanks,
David

@openjdk
Copy link

openjdk bot commented Apr 29, 2021

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

8262376: ReplaceCriticalClassesForSubgraphs.java fails if --with-build-jdk is used

Reviewed-by: dholmes, minqi, ccheung

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 29 new commits pushed to the master branch:

  • 49d0458: 8266288: assert root method not found in witnessed_reabstraction_in_supers is too strong
  • 01415f3: 8266250: WebSocketTest and WebSocketProxyTest call assertEquals(List<byte[]>, List<byte[]>)
  • 5f15666: 8266078: Reader.read(CharBuffer) advances Reader position for read-only Charbuffers
  • 2a03739: 8266014: Regression brought by optimization done with JDK-4926314
  • 6bb71d9: 8264762: ByteBuffer.byteOrder(BIG_ENDIAN).asXBuffer.put(Xarray) and ByteBuffer.byteOrder(nativeOrder()).asXBuffer.put(Xarray) are slow
  • f0f6b0d: 8266027: The diamond finder does not find diamond candidates in field initializers
  • 8072ea5: 8238173: jshell - switch statement with a single default not return cause syntax error
  • c76ce28: 8265842: G1: Introduce API to run multiple separate tasks in a single gangtask
  • 294347b: 8265918: java/io/Console/CharsetTest.java failed with "expect: spawn id exp6 not open"
  • 84b52db: 8265666: Enable AIX build platform to make external debug symbols
  • ... and 19 more: https://git.openjdk.java.net/jdk/compare/23180f848f068434d018b741db6f34cd4b6da55d...master

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 openjdk bot added the ready Pull request is ready to be integrated label Apr 29, 2021
Copy link
Contributor

@yminqi yminqi left a comment

Choose a reason for hiding this comment

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

LGTM

@iklam
Copy link
Member Author

iklam commented Apr 29, 2021

Does this mean that when using CDS some classes are not subject to the application of the JVM TI ClassFileLoadHook? And if so, is that constraint documented somewhere?

Hi David, thanks for the review.

CDS tries to be transparent w.r.t. ClassFileLoadHook:

  • If JVMTI "early" ClassFileLoadHooks are enabled, CDS is disabled.
  • If only JVMTI "non-early" ClassFileLoadHooks are enabled, they will get all the class loading events that happen after the VM has finished the early phase. (This is the same whether CDS is enabled or not).

Since the module graph is built during the early phase, a "non-early" ClassFileLoadHook cannot expect to get the loading events for the classes that were loaded in order to build the module graph.

The exact set of such classes depend on the implementation of the module system, so it could change between JDK versions. This change only affects two internal classes - sun.net.www.protocol.jrt.Handler and jdk.internal.module.SystemModuleFinders$SystemModuleReader, so I think the impact is minimal.

@AlanBateman
Copy link
Contributor

Does this mean that when using CDS some classes are not subject to the application of the JVM TI ClassFileLoadHook? And if so, is that constraint documented somewhere?

At one point, enabling the can_generate_all_class_hook_events capability in the onload phase would disable CDS but maybe that has changed? In any case, the only place in the specs where it touches on this is in the CFLH event itself where it has a sentence about not generating the event for some classes.

@mlbridge
Copy link

mlbridge bot commented Apr 29, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 29/04/2021 2:15 pm, Ioi Lam wrote:

On Thu, 29 Apr 2021 01:41:34 GMT, David Holmes <dholmes at openjdk.org> wrote:

Does this mean that when using CDS some classes are not subject to the application of the JVM TI ClassFileLoadHook? And if so, is that constraint documented somewhere?

Hi David, thanks for the review.

CDS tries to be transparent w.r.t. ClassFileLoadHook:

- If JVMTI "early" ClassFileLoadHooks are enabled, CDS is disabled.
- If only JVMTI "non-early" ClassFileLoadHooks are enabled, they will get all the class loading events that happen after the VM has finished the early phase. (This is the same whether CDS is enabled or not).

Since the module graph is built during the early phase, a "non-early" ClassFileLoadHook cannot expect to get the loading events for the classes that were loaded in order to build the module graph.

The exact set of such classes depend on the implementation of the module system, so it could change between JDK versions. This change only affects two internal classes - sun.net.www.protocol.jrt.Handler and jdk.internal.module.SystemModuleFinders$SystemModuleReader, so I think the impact is minimal.

Thanks for clarifying, I had forgotten that the module system had
introduced two phases for the CFLH.

David

Copy link
Member

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

@iklam
Copy link
Member Author

iklam commented Apr 29, 2021

Does this mean that when using CDS some classes are not subject to the application of the JVM TI ClassFileLoadHook? And if so, is that constraint documented somewhere?

At one point, enabling the can_generate_all_class_hook_events capability in the onload phase would disable CDS but maybe that has changed? In any case, the only place in the specs where it touches on this is in the CFLH event itself where it has a sentence about not generating the event for some classes.

CDS started supporting ClassFileLoadHook since JDK-8078644 (JDK9). At that time, CLFH was enabled for all shared classes.

However, since JDK-8212200 (JDK12), we disable CDS when JvmtiExport::has_early_class_hook_env() == true.

@iklam
Copy link
Member Author

iklam commented Apr 29, 2021

Thanks @calvinccheung @dholmes-ora @yminqi for the review!
/integrate

@openjdk openjdk bot closed this Apr 29, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 29, 2021
@openjdk
Copy link

openjdk bot commented Apr 29, 2021

@iklam Since your change was applied there have been 35 commits pushed to the master branch:

  • 5ecef01: 8266217: ZGC: Improve the -Xlog:gc+init output for NUMA
  • 5d8c1cc: 8255410: Add ChaCha20 and Poly1305 support to SunPKCS11 provider
  • 46b4a14: 8266315: Problem list failing test java/awt/font/TextLayout/LigatureCaretTest.java
  • 42af7da: 8265933: Move Java monitor related fields from class Thread to JavaThread
  • 1afbab6: 8263998: Remove mentions of mc region in comments
  • 51b2fb5: 8266299: ProblemList runtime/stringtable/StringTableCleaningTest.java on linux-aarch64 with ZGC
  • 49d0458: 8266288: assert root method not found in witnessed_reabstraction_in_supers is too strong
  • 01415f3: 8266250: WebSocketTest and WebSocketProxyTest call assertEquals(List<byte[]>, List<byte[]>)
  • 5f15666: 8266078: Reader.read(CharBuffer) advances Reader position for read-only Charbuffers
  • 2a03739: 8266014: Regression brought by optimization done with JDK-4926314
  • ... and 25 more: https://git.openjdk.java.net/jdk/compare/23180f848f068434d018b741db6f34cd4b6da55d...master

Your commit was automatically rebased without conflicts.

Pushed as commit 2c381e0.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

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