Skip to content

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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 34 additions & 11 deletions src/hotspot/share/classfile/stringTable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#include "classfile/javaClasses.inline.hpp"
#include "classfile/stringTable.hpp"
#include "classfile/vmClasses.hpp"
#include "compiler/compileBroker.hpp"
#include "gc/shared/collectedHeap.hpp"
#include "gc/shared/oopStorage.inline.hpp"
#include "gc/shared/oopStorageSet.hpp"
Expand Down Expand Up @@ -115,6 +116,7 @@ OopStorage* StringTable::_oop_storage;

static size_t _current_size = 0;
static volatile size_t _items_count = 0;
DEBUG_ONLY(static int _disable_interning_during_cds_dump = 0);

volatile bool _alt_hash = false;

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

size_t StringTable::items_count() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

return Atomic::load_acquire(&_items_count);
}

void StringTable::trigger_concurrent_work() {
// Avoid churn on ServiceThread
if (!has_work()) {
Expand Down Expand Up @@ -504,6 +510,9 @@ oop StringTable::intern(const char* utf8_string, TRAPS) {
}

oop StringTable::intern(const StringWrapper& name, TRAPS) {
assert(!Atomic::load_acquire(&_disable_interning_during_cds_dump),
"All threads that may intern strings should have been stopped before CDS starts copying the interned string table");

// shared table always uses java_lang_String::hash_code
unsigned int hash = hash_wrapped_string(name);
oop found_string = lookup_shared(name, hash);
Expand Down Expand Up @@ -793,7 +802,7 @@ void StringTable::verify() {
}

// Verification and comp
class VerifyCompStrings : StackObj {
class StringTable::VerifyCompStrings : StackObj {
static unsigned string_hash(oop const& str) {
return java_lang_String::hash_code_noupdate(str);
}
Expand All @@ -805,7 +814,7 @@ class VerifyCompStrings : StackObj {
string_hash, string_equals> _table;
public:
size_t _errors;
VerifyCompStrings() : _table(unsigned(_items_count / 8) + 1, 0 /* do not resize */), _errors(0) {}
VerifyCompStrings() : _table(unsigned(items_count() / 8) + 1, 0 /* do not resize */), _errors(0) {}
bool operator()(WeakHandle* val) {
oop s = val->resolve();
if (s == nullptr) {
Expand Down Expand Up @@ -939,20 +948,32 @@ oop StringTable::lookup_shared(const jchar* name, int len) {
return _shared_table.lookup(wrapped_name, java_lang_String::hash_code(name, len), 0);
}

// This is called BEFORE we enter the CDS safepoint. We can allocate heap objects.
// This should be called when we know no more strings will be added (which will be easy
// to guarantee because CDS runs with a single Java thread. See JDK-8253495.)
// This is called BEFORE we enter the CDS safepoint. We can allocate still Java object arrays to
// be used by the shared strings table.
void StringTable::allocate_shared_strings_array(TRAPS) {
if (!CDSConfig::is_dumping_heap()) {
return;
}
assert(CDSConfig::allow_only_single_java_thread(), "No more interned strings can be added");

if (_items_count > (size_t)max_jint) {
fatal("Too many strings to be archived: %zu", _items_count);
CompileBroker::wait_for_no_active_tasks();

precond(THREAD->is_Java_thread());
precond(CDSConfig::allow_only_single_java_thread());

// At this point, no more strings will be added:
// - There's only a single Java thread (this thread). It no longer executes Java bytecodes
// so JIT compilation will eventually stop.
// - CompileBroker has no more active tasks, so all JIT requests have been processed.

// 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));
Copy link
Contributor

Choose a reason for hiding this comment

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

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


if (items_count() > (size_t)max_jint) {
fatal("Too many strings to be archived: %zu", items_count());
}

int total = (int)_items_count;
int total = (int)items_count();
size_t single_array_size = objArrayOopDesc::object_size(total);

log_info(aot)("allocated string table for %d strings", total);
Expand All @@ -972,7 +993,7 @@ void StringTable::allocate_shared_strings_array(TRAPS) {
// This can only happen if you have an extremely large number of classes that
// refer to more than 16384 * 16384 = 26M interned strings! Not a practical concern
// but bail out for safety.
log_error(aot)("Too many strings to be archived: %zu", _items_count);
log_error(aot)("Too many strings to be archived: %zu", items_count());
MetaspaceShared::unrecoverable_writing_error();
}

Expand Down Expand Up @@ -1070,7 +1091,7 @@ oop StringTable::init_shared_strings_array() {

void StringTable::write_shared_table() {
_shared_table.reset();
CompactHashtableWriter writer((int)_items_count, ArchiveBuilder::string_stats());
CompactHashtableWriter writer((int)items_count(), ArchiveBuilder::string_stats());

int index = 0;
auto copy_into_shared_table = [&] (WeakHandle* val) {
Expand All @@ -1084,6 +1105,8 @@ void StringTable::write_shared_table() {
};
_local_table->do_safepoint_scan(copy_into_shared_table);
writer.dump(&_shared_table, "string");

DEBUG_ONLY(Atomic::release_store(&_disable_interning_during_cds_dump, 0));
}

void StringTable::set_shared_strings_array_index(int root_index) {
Expand Down
3 changes: 2 additions & 1 deletion src/hotspot/share/classfile/stringTable.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class StringTableConfig;

class StringTable : AllStatic {
friend class StringTableConfig;

class VerifyCompStrings;
static volatile bool _has_work;

// Set if one bucket is out of balance due to hash algorithm deficiency
Expand Down Expand Up @@ -74,6 +74,7 @@ class StringTable : AllStatic {

static void item_added();
static void item_removed();
static size_t items_count();

static oop intern(const StringWrapper& name, TRAPS);
static oop do_intern(const StringWrapper& name, uintx hash, TRAPS);
Expand Down
4 changes: 4 additions & 0 deletions src/hotspot/share/compiler/compileBroker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1750,6 +1750,10 @@ void CompileBroker::wait_for_completion(CompileTask* task) {
}
}

void CompileBroker::wait_for_no_active_tasks() {
CompileTask::wait_for_no_active_tasks();
}

/**
* Initialize compiler thread(s) + compiler object(s). The postcondition
* of this function is that the compiler runtimes are initialized and that
Expand Down
3 changes: 3 additions & 0 deletions src/hotspot/share/compiler/compileBroker.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,9 @@ class CompileBroker: AllStatic {
static bool is_compilation_disabled_forever() {
return _should_compile_new_jobs == shutdown_compilation;
}

static void wait_for_no_active_tasks();

static void handle_full_code_cache(CodeBlobType code_blob_type);
// Ensures that warning is only printed once.
static bool should_print_compiler_warning() {
Expand Down
17 changes: 15 additions & 2 deletions src/hotspot/share/compiler/compileTask.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,13 @@
#include "runtime/mutexLocker.hpp"

CompileTask* CompileTask::_task_free_list = nullptr;
int CompileTask::_active_tasks = 0;

/**
* Allocate a CompileTask, from the free list if possible.
*/
CompileTask* CompileTask::allocate() {
MutexLocker locker(CompileTaskAlloc_lock);
MonitorLocker locker(CompileTaskAlloc_lock);
CompileTask* task = nullptr;

if (_task_free_list != nullptr) {
Expand All @@ -56,14 +57,15 @@ CompileTask* CompileTask::allocate() {
}
assert(task->is_free(), "Task must be free.");
task->set_is_free(false);
_active_tasks++;
return task;
}

/**
* Add a task to the free list.
*/
void CompileTask::free(CompileTask* task) {
MutexLocker locker(CompileTaskAlloc_lock);
MonitorLocker locker(CompileTaskAlloc_lock);
if (!task->is_free()) {
if ((task->_method_holder != nullptr && JNIHandles::is_weak_global_handle(task->_method_holder))) {
JNIHandles::destroy_weak_global(task->_method_holder);
Expand All @@ -79,6 +81,17 @@ void CompileTask::free(CompileTask* task) {
task->set_is_free(true);
task->set_next(_task_free_list);
_task_free_list = task;
_active_tasks--;
if (_active_tasks == 0) {
locker.notify_all();
}
}
}

void CompileTask::wait_for_no_active_tasks() {
MonitorLocker locker(CompileTaskAlloc_lock);
while (_active_tasks > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

locker.wait();
}
}

Expand Down
4 changes: 3 additions & 1 deletion src/hotspot/share/compiler/compileTask.hpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1998, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1998, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -83,6 +83,7 @@ class CompileTask : public CHeapObj<mtCompiler> {

private:
static CompileTask* _task_free_list;
static int _active_tasks;
int _compile_id;
Method* _method;
jobject _method_holder;
Expand Down Expand Up @@ -123,6 +124,7 @@ class CompileTask : public CHeapObj<mtCompiler> {

static CompileTask* allocate();
static void free(CompileTask* task);
static void wait_for_no_active_tasks();

int compile_id() const { return _compile_id; }
Method* method() const { return _method; }
Expand Down
4 changes: 2 additions & 2 deletions src/hotspot/share/runtime/mutexLocker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ Monitor* CompileTaskWait_lock = nullptr;
Monitor* MethodCompileQueue_lock = nullptr;
Monitor* CompileThread_lock = nullptr;
Monitor* Compilation_lock = nullptr;
Mutex* CompileTaskAlloc_lock = nullptr;
Monitor* CompileTaskAlloc_lock = nullptr;
Mutex* CompileStatistics_lock = nullptr;
Mutex* DirectivesStack_lock = nullptr;
Monitor* Terminator_lock = nullptr;
Expand Down Expand Up @@ -343,7 +343,7 @@ void mutex_init() {
MUTEX_DEFL(G1RareEvent_lock , PaddedMutex , Threads_lock, true);
}

MUTEX_DEFL(CompileTaskAlloc_lock , PaddedMutex , MethodCompileQueue_lock);
MUTEX_DEFL(CompileTaskAlloc_lock , PaddedMonitor, MethodCompileQueue_lock);
MUTEX_DEFL(CompileTaskWait_lock , PaddedMonitor, MethodCompileQueue_lock);

#if INCLUDE_PARALLELGC
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/runtime/mutexLocker.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ extern Monitor* CompileThread_lock; // a lock held by compile threa
extern Monitor* Compilation_lock; // a lock used to pause compilation
extern Mutex* TrainingData_lock; // a lock used when accessing training records
extern Monitor* TrainingReplayQueue_lock; // a lock held when class are added/removed to the training replay queue
extern Mutex* CompileTaskAlloc_lock; // a lock held when CompileTasks are allocated
extern Monitor* CompileTaskAlloc_lock; // a lock held when CompileTasks are allocated
extern Monitor* CompileTaskWait_lock; // a lock held when CompileTasks are waited/notified
extern Mutex* CompileStatistics_lock; // a lock held when updating compilation statistics
extern Mutex* DirectivesStack_lock; // a lock held when mutating the dirstack and ref counting directives
Expand Down