8320826: call allocate_shared_strings_array after all strings are interned

[iklam]

StringTable::allocate_shared_strings_array() creates a Java object array that has the exact same size as the number of currently interned strings. Therefore, we need to move it to just before we enter the safepoint, where we know no more Java code would be execute.

I also had to change the assert in StringTable::allocate_shared_strings_array() to a condition check: it's possible for HeapShared::init_for_dumping() to disable heap dumping when a Java agent transforms classes like jdk.internal.math.FDBigInteger.

---

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-8320826: call allocate_shared_strings_array after all strings are interned (Bug - P4)

Reviewers

• Calvin Cheung (@calvinccheung - Reviewer)
• Matias Saavedra Silva (@matias9927 - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16839

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16839.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 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]

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

[mlbridge]

Webrevs

• 00: Full (b22174c7)

[calvinccheung]

LGTM

[openjdk]

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

8320826: call allocate_shared_strings_array after all strings are interned

Reviewed-by: ccheung, matsaave

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

• da09eab: 8319980: [JVMCI] libgraal should reuse Thread instances as C2 does
• 33b26f7: 8319123: Implement JEP 461: Stream Gatherers (Preview)
• 04ad98e: 8315458: Implement JEP 463: Implicitly Declared Classes and Instance Main Method (Second Preview)
• 03759e8: 8320304: Refactor and simplify monitor deflation functions
• da7cf25: 8320665: update jdk_core at open/test/jdk/TEST.groups
• c9d15f7: 8321025: Enable Neoverse N1 optimizations for Neoverse V2
• 61653a1: 8320830: [AIX] Dont mix os::dll_load() with direct dlclose() calls
• 8b102ed: 8321063: AArch64: Zero build fails after JDK-8320368
• 6941369: 8320921: GHA: Parallelize hotspot_compiler test jobs
• 0d14636: 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object
• ... and 73 more: https://git.openjdk.org/jdk/compare/6aa197667ad05bd93adf3afc7b06adbfb2b18a22...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.

[matias9927]

Looks good! I just have one concern below:

[matias9927]

This seems to be redundant considering that allocate_shared_strings_array() is only called by preload_and_dump_impl. Unless you expect this method to be called elsewhere eventually?

[iklam]

At the point where preload_and_dump_impl() calls allocate_shared_strings_array(), heap dumping may have been disabled. So this check avoids doing unnecessary work for that situation.

[matias9927]

Thanks for the clarification, approved

[iklam]

Thanks @calvinccheung and @matias9927 for the review.
/integrate

[openjdk]

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

• 0a60b0f: 8302233: HSS/LMS: keytool and jarsigner changes
• 7ad7005: 8315034: File.mkdirs() occasionally fails to create folders on Windows shared folder
• 41daa3b: 8320239: add dynamic switch for JvmtiVTMSTransitionDisabler sync protocol
• 7c135c3: 8321066: Multiple JFR tests have started failing
• 8bedb28: 8321119: Disable java/foreign/TestHandshake.java on Zero VMs
• b1cbf55: 8321018: Parallel: Make some methods in ParCompactionManager private
• c6a8278: 8321127: ProblemList java/util/stream/GatherersTest.java
• a3eb664: 8315701: [macos] Regression: KeyEvent has different keycode on different keyboard layouts
• 6aba6aa: 8320347: Emulate vblendvp[sd] on ECore
• 6938474: 8320916: jdk/jfr/event/gc/stacktrace/TestParallelMarkSweepAllocationPendingStackTrace.java failed with "OutOfMemoryError: GC overhead limit exceeded"
• ... and 83 more: https://git.openjdk.org/jdk/compare/6aa197667ad05bd93adf3afc7b06adbfb2b18a22...master

Your commit was automatically rebased without conflicts.

[openjdk]

@iklam Pushed as commit 630bafd.

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