8356125: Interned strings are omitted from AOT cache

Reviewed-by: shade, ccheung

Author: iklam

src/hotspot/share/cds/aotClassLinker.cpp

@@ -64,8 +64,6 @@ void AOTClassLinker::initialize() {
   }
 
   assert(is_initialized(), "sanity");
-
-  AOTConstantPoolResolver::initialize();
 }
 
 void AOTClassLinker::dispose() {
@@ -79,8 +77,6 @@ void AOTClassLinker::dispose() {
   _sorted_candidates = nullptr;
 
   assert(!is_initialized(), "sanity");
-
-  AOTConstantPoolResolver::dispose();
 }
 
 bool AOTClassLinker::is_vm_class(InstanceKlass* ik) {

src/hotspot/share/cds/aotConstantPoolResolver.cpp

@@ -37,19 +37,6 @@
 #include "oops/klass.inline.hpp"
 #include "runtime/handles.inline.hpp"
 
-AOTConstantPoolResolver::ClassesTable* AOTConstantPoolResolver::_processed_classes = nullptr;
-
-void AOTConstantPoolResolver::initialize() {
-  assert(_processed_classes == nullptr, "must be");
-  _processed_classes = new (mtClass)ClassesTable();
-}
-
-void AOTConstantPoolResolver::dispose() {
-  assert(_processed_classes != nullptr, "must be");
-  delete _processed_classes;
-  _processed_classes = nullptr;
-}
-
 // Returns true if we CAN PROVE that cp_index will always resolve to
 // the same information at both dump time and run time. This is a
 // necessary (but not sufficient) condition for pre-resolving cp_index
@@ -144,17 +131,11 @@ bool AOTConstantPoolResolver::is_class_resolution_deterministic(InstanceKlass* c
   }
 }
 
-void AOTConstantPoolResolver::dumptime_resolve_constants(InstanceKlass* ik, TRAPS) {
+void AOTConstantPoolResolver::preresolve_string_cp_entries(InstanceKlass* ik, TRAPS) {
   if (!ik->is_linked()) {
+    // The cp->resolved_referenced() array is not ready yet, so we can't call resolve_string().
     return;
   }
-  bool first_time;
-  _processed_classes->put_if_absent(ik, &first_time);
-  if (!first_time) {
-    // We have already resolved the constants in class, so no need to do it again.
-    return;
-  }
-
   constantPoolHandle cp(THREAD, ik->constants());
   for (int cp_index = 1; cp_index < cp->length(); cp_index++) { // Index 0 is unused
     switch (cp->tag_at(cp_index).value()) {

src/hotspot/share/cds/aotConstantPoolResolver.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022, 2024, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2022, 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
@@ -80,17 +80,10 @@ class AOTConstantPoolResolver :  AllStatic {
   static bool check_lambda_metafactory_methodhandle_arg(ConstantPool* cp, int bsms_attribute_index, int arg_i);
 
 public:
-  static void initialize();
-  static void dispose();
-
   static void preresolve_class_cp_entries(JavaThread* current, InstanceKlass* ik, GrowableArray<bool>* preresolve_list);
   static void preresolve_field_and_method_cp_entries(JavaThread* current, InstanceKlass* ik, GrowableArray<bool>* preresolve_list);
   static void preresolve_indy_cp_entries(JavaThread* current, InstanceKlass* ik, GrowableArray<bool>* preresolve_list);
-
-
-  // Resolve all constant pool entries that are safe to be stored in the
-  // CDS archive.
-  static void dumptime_resolve_constants(InstanceKlass* ik, TRAPS);
+  static void preresolve_string_cp_entries(InstanceKlass* ik, TRAPS);
 
   static bool is_resolution_deterministic(ConstantPool* cp, int cp_index);
 };

src/hotspot/share/cds/heapShared.cpp

@@ -606,11 +606,8 @@ static objArrayOop get_archived_resolved_references(InstanceKlass* src_ik) {
 }
 
 void HeapShared::archive_strings() {
-  oop shared_strings_array = StringTable::init_shared_strings_array(_dumped_interned_strings);
+  oop shared_strings_array = StringTable::init_shared_strings_array();
   bool success = archive_reachable_objects_from(1, _dump_time_special_subgraph, shared_strings_array);
-  // We must succeed because:
-  // - _dumped_interned_strings do not contain any large strings.
-  // - StringTable::init_shared_table() doesn't create any large arrays.
   assert(success, "shared strings array must not point to arrays or strings that are too large to archive");
   StringTable::set_shared_strings_array_index(append_root(shared_strings_array));
 }
@@ -683,7 +680,7 @@ void HeapShared::write_heap(ArchiveHeapInfo *heap_info) {
     check_special_subgraph_classes();
   }
 
-  StringTable::write_shared_table(_dumped_interned_strings);
+  StringTable::write_shared_table();
   ArchiveHeapWriter::write(_pending_roots, heap_info);
 
   ArchiveBuilder::OtherROAllocMark mark;
@@ -710,8 +707,6 @@ void HeapShared::scan_java_class(Klass* orig_k) {
       bool success = HeapShared::archive_reachable_objects_from(1, _dump_time_special_subgraph, rr);
       assert(success, "must be");
     }
-
-    orig_ik->constants()->add_dumped_interned_strings();
   }
 }
 
@@ -2067,11 +2062,8 @@ void HeapShared::archive_object_subgraphs(ArchivableStaticFieldInfo fields[],
 #endif
 }
 
-// Not all the strings in the global StringTable are dumped into the archive, because
-// some of those strings may be only referenced by classes that are excluded from
-// the archive. We need to explicitly mark the strings that are:
-//   [1] used by classes that WILL be archived;
-//   [2] included in the SharedArchiveConfigFile.
+// Keep track of the contents of the archived interned string table. This table
+// is used only by CDSHeapVerifier.
 void HeapShared::add_to_dumped_interned_strings(oop string) {
   assert_at_safepoint(); // DumpedInternedStrings uses raw oops
   assert(!ArchiveHeapWriter::is_string_too_large_to_archive(string), "must be");

src/hotspot/share/cds/metaspaceShared.cpp

@@ -646,15 +646,6 @@ void VM_PopulateDumpSharedSpace::doit() {
   // Block concurrent class unloading from changing the _dumptime_table
   MutexLocker ml(DumpTimeTable_lock, Mutex::_no_safepoint_check_flag);
 
-#if INCLUDE_CDS_JAVA_HEAP
-  if (CDSConfig::is_dumping_heap() && _extra_interned_strings != nullptr) {
-    for (int i = 0; i < _extra_interned_strings->length(); i ++) {
-      OopHandle string = _extra_interned_strings->at(i);
-      HeapShared::add_to_dumped_interned_strings(string.resolve());
-    }
-  }
-#endif
-
   _builder.gather_source_objs();
   _builder.reserve_buffer();
 
@@ -777,15 +768,15 @@ void MetaspaceShared::link_shared_classes(TRAPS) {
     // Keep scanning until we have linked no more classes.
   }
 
-  // Resolve constant pool entries -- we don't load any new classes during this stage
+  // Eargerly resolve all string constants in constant pools
   {
     ResourceMark rm(THREAD);
     CollectClassesForLinking collect_classes;
     const GrowableArray<OopHandle>* mirrors = collect_classes.mirrors();
     for (int i = 0; i < mirrors->length(); i++) {
       OopHandle mirror = mirrors->at(i);
       InstanceKlass* ik = InstanceKlass::cast(java_lang_Class::as_Klass(mirror.resolve()));
-      AOTConstantPoolResolver::dumptime_resolve_constants(ik, CHECK);
+      AOTConstantPoolResolver::preresolve_string_cp_entries(ik, CHECK);
     }
   }
 

src/hotspot/share/classfile/stringTable.cpp

@@ -946,6 +946,8 @@ 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);
   }
@@ -1026,48 +1028,61 @@ void StringTable::verify_secondary_array_index_bits() {
 // For each shared string:
 // [1] Store it into _shared_strings_array. Encode its position as a 32-bit index.
 // [2] Store the index and hashcode into _shared_table.
-oop StringTable::init_shared_strings_array(const DumpedInternedStrings* dumped_interned_strings) {
+oop StringTable::init_shared_strings_array() {
   assert(CDSConfig::is_dumping_heap(), "must be");
   objArrayOop array = (objArrayOop)(_shared_strings_array.resolve());
 
   verify_secondary_array_index_bits();
 
   int index = 0;
-  auto copy_into_array = [&] (oop string, bool value_ignored) {
-    if (!_is_two_dimensional_shared_strings_array) {
-      assert(index < array->length(), "no strings should have been added");
-      array->obj_at_put(index, string);
-    } else {
-      int primary_index = index >> _secondary_array_index_bits;
-      int secondary_index = index & _secondary_array_index_mask;
+  auto copy_into_array = [&] (WeakHandle* val) {
+    oop string = val->peek();
+    if (string != nullptr && !ArchiveHeapWriter::is_string_too_large_to_archive(string)) {
+      // If string is too large, don't put it into the string table.
+      // - If there are no other refernences to it, it won't be stored into the archive,
+      //   so we are all good.
+      // - If there's a referece to it, we will report an error inside HeapShared.cpp and
+      //   dumping will fail.
+      HeapShared::add_to_dumped_interned_strings(string);
+      if (!_is_two_dimensional_shared_strings_array) {
+        assert(index < array->length(), "no strings should have been added");
+        array->obj_at_put(index, string);
+      } else {
+        int primary_index = index >> _secondary_array_index_bits;
+        int secondary_index = index & _secondary_array_index_mask;
 
-      assert(primary_index < array->length(), "no strings should have been added");
-      objArrayOop secondary = (objArrayOop)array->obj_at(primary_index);
+        assert(primary_index < array->length(), "no strings should have been added");
+        objArrayOop secondary = (objArrayOop)array->obj_at(primary_index);
 
-      assert(secondary != nullptr && secondary->is_objArray(), "must be");
-      assert(secondary_index < secondary->length(), "no strings should have been added");
-      secondary->obj_at_put(secondary_index, string);
+        assert(secondary != nullptr && secondary->is_objArray(), "must be");
+        assert(secondary_index < secondary->length(), "no strings should have been added");
+        secondary->obj_at_put(secondary_index, string);
+      }
+      index ++;
     }
-
-    index ++;
+    return true;
   };
-  dumped_interned_strings->iterate_all(copy_into_array);
 
+  _local_table->do_safepoint_scan(copy_into_array);
+  log_info(cds)("Archived %d interned strings", index);
   return array;
-}
+};
 
-void StringTable::write_shared_table(const DumpedInternedStrings* dumped_interned_strings) {
+void StringTable::write_shared_table() {
   _shared_table.reset();
   CompactHashtableWriter writer((int)_items_count, ArchiveBuilder::string_stats());
 
   int index = 0;
-  auto copy_into_shared_table = [&] (oop string, bool value_ignored) {
-    unsigned int hash = java_lang_String::hash_code(string);
-    writer.add(hash, index);
-    index ++;
+  auto copy_into_shared_table = [&] (WeakHandle* val) {
+    oop string = val->peek();
+    if (string != nullptr && !ArchiveHeapWriter::is_string_too_large_to_archive(string)) {
+      unsigned int hash = java_lang_String::hash_code(string);
+      writer.add(hash, index);
+      index ++;
+    }
+    return true;
   };
-  dumped_interned_strings->iterate_all(copy_into_shared_table);
-
+  _local_table->do_safepoint_scan(copy_into_shared_table);
   writer.dump(&_shared_table, "string");
 }
 

src/hotspot/share/classfile/stringTable.hpp

@@ -33,7 +33,6 @@
 #include "utilities/tableStatistics.hpp"
 
 class CompactHashtableWriter;
-class DumpedInternedStrings;
 class JavaThread;
 class SerializeClosure;
 
@@ -147,8 +146,8 @@ class StringTable : AllStatic {
   static oop lookup_shared(const jchar* name, int len) NOT_CDS_JAVA_HEAP_RETURN_(nullptr);
   static size_t shared_entry_count() NOT_CDS_JAVA_HEAP_RETURN_(0);
   static void allocate_shared_strings_array(TRAPS) NOT_CDS_JAVA_HEAP_RETURN;
-  static oop init_shared_strings_array(const DumpedInternedStrings* dumped_interned_strings) NOT_CDS_JAVA_HEAP_RETURN_(nullptr);
-  static void write_shared_table(const DumpedInternedStrings* dumped_interned_strings) NOT_CDS_JAVA_HEAP_RETURN;
+  static oop init_shared_strings_array() NOT_CDS_JAVA_HEAP_RETURN_(nullptr);
+  static void write_shared_table() NOT_CDS_JAVA_HEAP_RETURN;
   static void set_shared_strings_array_index(int root_index) NOT_CDS_JAVA_HEAP_RETURN;
   static void serialize_shared_table_header(SerializeClosure* soc) NOT_CDS_JAVA_HEAP_RETURN;
 

src/hotspot/share/oops/constantPool.cpp

@@ -374,40 +374,6 @@ objArrayOop ConstantPool::prepare_resolved_references_for_archiving() {
   }
   return rr;
 }
-
-void ConstantPool::add_dumped_interned_strings() {
-  InstanceKlass* ik = pool_holder();
-  if (!ik->is_linked()) {
-    // resolved_references() doesn't exist yet, so we have no resolved CONSTANT_String entries. However,
-    // some static final fields may have default values that were initialized when the class was parsed.
-    // We need to enter those into the CDS archive strings table.
-    for (JavaFieldStream fs(ik); !fs.done(); fs.next()) {
-      if (fs.access_flags().is_static()) {
-        fieldDescriptor& fd = fs.field_descriptor();
-        if (fd.field_type() == T_OBJECT) {
-          int offset = fd.offset();
-          check_and_add_dumped_interned_string(ik->java_mirror()->obj_field(offset));
-        }
-      }
-    }
-  } else {
-    objArrayOop rr = resolved_references();
-    if (rr != nullptr) {
-      int rr_len = rr->length();
-      for (int i = 0; i < rr_len; i++) {
-        check_and_add_dumped_interned_string(rr->obj_at(i));
-      }
-    }
-  }
-}
-
-void ConstantPool::check_and_add_dumped_interned_string(oop obj) {
-  if (obj != nullptr && java_lang_String::is_instance(obj) &&
-      !ArchiveHeapWriter::is_string_too_large_to_archive(obj)) {
-    HeapShared::add_to_dumped_interned_strings(obj);
-  }
-}
-
 #endif
 
 #if INCLUDE_CDS

src/hotspot/share/oops/constantPool.hpp

@@ -162,7 +162,6 @@ class ConstantPool : public Metadata {
     assert(is_within_bounds(cp_index), "index out of bounds");
     return (jdouble*) &base()[cp_index];
   }
-  static void check_and_add_dumped_interned_string(oop obj);
 
   ConstantPool(Array<u1>* tags);
   ConstantPool();
@@ -684,7 +683,6 @@ class ConstantPool : public Metadata {
 #if INCLUDE_CDS
   // CDS support
   objArrayOop prepare_resolved_references_for_archiving() NOT_CDS_JAVA_HEAP_RETURN_(nullptr);
-  void add_dumped_interned_strings() NOT_CDS_JAVA_HEAP_RETURN;
   void remove_unshareable_info();
   void restore_unshareable_info(TRAPS);
 private: