8358680: AOT cache creation fails: no strings should have been added #25816

iklam · 2025-06-16T03:30:40Z

Background: when writing the string table in the AOT cache, we do this:

Find out the number of strings in the interned string table
Allocate Java object arrays that are large enough to store these strings. These arrays are used by StringTable::lookup_shared() in the production run.
Enter safepoint
Copy the strings into the arrays

This bug happened because:

Step 1 is not thread safe, so it may be reading a stale version of _items_count
JIT compiler threads may create more interned strings after step 1

This PR attempts to fix both issues.

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-8358680: AOT cache creation fails: no strings should have been added (Bug - P4)

Contributors

Aleksey Shipilev <[email protected]>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25816

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-06-16T03:31:15Z

👋 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 · 2025-06-16T03:31:31Z

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

openjdk · 2025-06-16T03:31:58Z

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

mlbridge · 2025-06-16T03:34:44Z

Webrevs

00: Full (de7ce83f)

iklam · 2025-06-16T03:37:04Z

@shipilev I copied the part of your JDK-8352042: [leyden] Parallel precompilation changes from the Leyden repo that implements CompileBroker::wait_for_no_active_tasks().

iklam · 2025-06-16T03:37:58Z

/contributor add @shipilev

openjdk · 2025-06-16T03:38:06Z

@iklam
Contributor Aleksey Shipilev <[email protected]> successfully added.

vnkozlov · 2025-06-16T05:05:12Z

Can something else add string to table between you get number and safe point? How you avoid that?
I assume you can't get number at safepoint because you need to allocate object array not at safepoint. Right?

iklam · 2025-06-16T06:08:30Z

Can something else add string to table between you get number and safe point? How you avoid that? I assume you can't get number at safepoint because you need to allocate object array not at safepoint. Right?

Not that I am aware of. In testing, I added precond(THREAD->is_Java_thread() into StringTable::intern() and it never failed. So I assume that this function can only be called by real Java threads or compiler threads.

iklam · 2025-06-16T06:17:57Z

Can something else add string to table between you get number and safe point? How you avoid that? I assume you can't get number at safepoint because you need to allocate object array not at safepoint. Right?

Not that I am aware of. In testing, I added precond(THREAD->is_Java_thread() into StringTable::intern() and it never failed. So I assume that this function can only be called by real Java threads or compiler threads.

Here are all the subclasses of JavaThread:

class AttachListenerThread : public JavaThread {
class CompilerThread : public JavaThread {
class DeoptimizeObjectsALotThread : public JavaThread {
class JvmtiAgentThread : public JavaThread {
class MonitorDeflationThread : public JavaThread {
class NotificationThread : public JavaThread {
class ServiceThread : public JavaThread {
class StringDedupThread : public JavaThread {
class TrainingReplayThread : public JavaThread {

StringDedupThread is disabled when dumping heap objects. And the other threads probably will either be inactive, or won't be doing anything that would result in string interning.

I guess someone could use jcmd to connect to the JVM at the wrong time and that might upset the archive assembly process. But that would create far worse problems than the bug that we are trying to fix here.

shipilev · 2025-06-16T10:57:19Z

@shipilev I copied the part of your JDK-8352042: [leyden] Parallel precompilation changes from the Leyden repo that implements CompileBroker::wait_for_no_active_tasks().

Well, I am rewriting that part in openjdk/leyden#79. I can give you a hunk on top of this PR that would do the same thing: use the CompileTaskWait_lock instead.

shipilev · 2025-06-16T17:29:24Z

Actually, I need #25409 in mainline first :)

iklam · 2025-06-16T21:00:17Z

Actually, I need #25409 in mainline first :)

I can wait for that for JDK 26.

If I want to fix for JDK 25, will the current fix be good enough?

shipilev · 2025-06-17T15:49:38Z

Would be nice to avoid divergence between JDK 25, JDK 26 and Leyden/premain.

Anyhow, I now think this fix in incomplete. In premain, we use this wait_for_no_active_tasks, because we know all the compiler tasks were queued, and we just need to run them down. But here, we still have a race: compiler threads may finish current batch of compilations, wait_for_no_active_tasks would return, and then we can start compiling again.

I think we need to figure our when we dump the shared table. Maybe even shutdown the compiler right before going into CDS dump? See how CompileBroker::set_should_block and VM_Exit::wait_for_threads_in_native_to_block do it. It would still be pretty awkward for a fix.

iklam · 2025-06-17T16:21:56Z

Would be nice to avoid divergence between JDK 25, JDK 26 and Leyden/premain.

Anyhow, I now think this fix in incomplete. In premain, we use this wait_for_no_active_tasks, because we know all the compiler tasks were queued, and we just need to run them down. But here, we still have a race: compiler threads may finish current batch of compilations, wait_for_no_active_tasks would return, and then we can start compiling again.

No Java code is executing at this point (we are in the only thread that can run Java code). Is there still a possibility for new compile tasks to be added?

I think we need to figure our when we dump the shared table. Maybe even shutdown the compiler right before going into CDS dump? See how CompileBroker::set_should_block and VM_Exit::wait_for_threads_in_native_to_block do it. It would still be pretty awkward for a fix.

In Leyden, we run the AOT compiler after the CDS dumping has finished, so if we shut down the compiler we would have to restart it.

coleenp

I have a couple of comments. Overall, the approach seems good.

coleenp · 2025-06-17T15:51:57Z

src/hotspot/share/classfile/stringTable.cpp

+
+  // This flag will be cleared after intern table dumping has completed, so we can run the
+  // compiler again (for future AOT method compilation, etc).
+  DEBUG_ONLY(Atomic::release_store(&_disable_interning_during_cds_dump, 1));


I think atomics work with bool or is this a refcount ?

coleenp · 2025-06-17T15:54:25Z

src/hotspot/share/compiler/compileTask.cpp

+
+void CompileTask::wait_for_no_active_tasks() {
+  MonitorLocker locker(CompileTaskAlloc_lock);
+  while (_active_tasks > 0) {


Doesn't this have to have an Atomic::load() to make it re-read in the loop? Even though it's after we reacquire the lock.

Not if _active_tasks is only written whilst the same lock is held.

coleenp · 2025-06-17T17:03:48Z

src/hotspot/share/classfile/stringTable.cpp

@@ -346,6 +348,10 @@ bool StringTable::has_work() {
  return Atomic::load_acquire(&_has_work);
 }

+size_t StringTable::items_count() {


I think there's a convention to make accessor functions that use acquire semantics to be named items_count_acquire().

shipilev · 2025-06-18T17:27:29Z

I still dislike hooking up to compiler infrastructure to figure out if something is adding interned strings. I really, really dislike the divergence we would introduce with JDK 25 -> JDK 26 once a variant of JDK-8357473 lands in mainline. I cannot yet think of better solution though, let me think about it some more. At very least we need to get the sequencing of patches right...

vnkozlov · 2025-06-18T17:53:53Z

I thought your comment was for #25841

8358680: AOT cache creation fails: no strings should have been added

de7ce83

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

openjdk bot added the hotspot [email protected] label Jun 16, 2025

coleenp suggested changes Jun 17, 2025

View reviewed changes

8358680: AOT cache creation fails: no strings should have been added #25816

Are you sure you want to change the base?

8358680: AOT cache creation fails: no strings should have been added #25816

Conversation

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

Uh oh!

Progress

Issue

Contributors

Reviewing

Uh oh!

bridgekeeper bot commented Jun 16, 2025

Uh oh!

openjdk bot commented Jun 16, 2025

Uh oh!

openjdk bot commented Jun 16, 2025

Uh oh!

mlbridge bot commented Jun 16, 2025

Webrevs

Uh oh!

iklam commented Jun 16, 2025

Uh oh!

iklam commented Jun 16, 2025

Uh oh!

openjdk bot commented Jun 16, 2025

Uh oh!

vnkozlov commented Jun 16, 2025

Uh oh!

iklam commented Jun 16, 2025

Uh oh!

iklam commented Jun 16, 2025

Uh oh!

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

Uh oh!

Uh oh!

shipilev commented Jun 16, 2025

Uh oh!

iklam commented Jun 16, 2025

Uh oh!

shipilev commented Jun 17, 2025

Uh oh!

iklam commented Jun 17, 2025

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

coleenp Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

coleenp Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

coleenp Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

shipilev commented Jun 18, 2025

Uh oh!

vnkozlov commented Jun 18, 2025

Uh oh!

Uh oh!

iklam commented Jun 16, 2025 •

edited by openjdk bot

Loading

shipilev commented Jun 16, 2025 •

edited

Loading