On Wed, 14 Jan 2026 15:05:24 GMT, Matias Saavedra Silva <[email protected]>
wrote:
> The alternate substitutability method relies on an injected Java object
> "acmp_maps" which is currently not being archived and thus leads to crashes
> attempting to run with a CDS archive. The issue stems from inline klasses
> being archived and thus loaded at dumptime while the acmp_maps oops is
> generated in the class file parser, leading this oop to be absent at runtime.
>
> Additionally, the other injected static field "null_reset" was not being
> archived either, so both of these fields are properly stored in the archived
> heap. In the case of CDS/AOT configurations where the heap is not dumped,
> acmp_maps is regenerated at class loading using a copy of the array stored in
> metadata.
>
> Tests and APIs are updated to conform to the new output generated by the use
> of acmp_maps and remove some test cases which target the old substitutability
> method.
This looks good but I have a couple of small comments.
src/hotspot/share/cds/aotConstantPoolResolver.cpp line 324:
> 322: is_static = " *** static";
> 323: break;
> 324:
if you restore this blank line, there are no changes to
aotConstantPoolResolver.cpp.
src/hotspot/share/cds/cdsHeapVerifier.cpp line 312:
> 310: // Any concrete value class will have a field ".null_reset"
> which holds an
> 311: // all-zero instance of the value class so it will not change
> between
> 312: // dump time and runtime.
I'm not sure how this comment about the .null_reset value applies in this if
statement? It doesn't seem related to the LIMITATION comment above and there
doesn't seem to be anything special done for null reset here either.
src/hotspot/share/cds/heapShared.cpp line 2004:
> 2002: Symbol* name = subgraph_k->name();
> 2003:
> 2004: if (subgraph_k->is_identity_class() &&
This seems like it's changed the meaning of this if statement. if it's an
instance_klass or not an identity class, then it'll miss an instance klass of
with the name Foo that shouldn't be archived. Is this the whitelist of klass
types of heap objects that are archived in the heap?
src/hotspot/share/classfile/classFileParser.cpp line 5558:
> 5556: map_h->int_at_put(i * 2 + 2,
> _layout_info->_nonoop_acmp_map->at(i).second);
> 5557:
> 5558: _acmp_maps_array->at_put(i * 2 + 1,
> _layout_info->_nonoop_acmp_map->at(i).first);
Suggestion:
// Also store acmp maps as metadata for regeneration when using dynamic
archive or AOT training data.
_acmp_maps_array->at_put(i * 2 + 1,
_layout_info->_nonoop_acmp_map->at(i).first);
src/hotspot/share/classfile/classFileParser.cpp line 5571:
> 5569:
> 5570: // Clear out this field so it doesn't get deallocated by the
> destructor
> 5571: _acmp_maps_array = nullptr;
This is right. It didn't look necessary but it is if
oopFactory::new_intArray() throws an OOM.
You could remove this field from ClassFileParser and this handling, and make it
a local variable if you reverse lines 5549 and 5550.
src/hotspot/share/oops/instanceKlass.cpp line 3142:
> 3140: constants()->restore_unshareable_info(CHECK);
> 3141:
> 3142: // restore acmp_maps java array from the version stored in metadata
Can you capitalize Restore and end the sentence with a period?
-------------
PR Review:
https://git.openjdk.org/valhalla/pull/1903#pullrequestreview-3672423018
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1903#discussion_r2699775490
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1903#discussion_r2699732522
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1903#discussion_r2699744867
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1903#discussion_r2699754083
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1903#discussion_r2699757647
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1903#discussion_r2699770231
