RFR: 8275731: CDS archived enums objects are recreated at runtime [v3]

[Ioi Lam]

On Mon, 17 Jan 2022 18:36:35 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> Ioi Lam has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
>> 
>>  - Merge branch 'master' into 8275731-heapshared-enum
>>  - added exclusions needed by "java -Xshare:dump -ea -esa"
>>  - Comments from @calvinccheung off-line
>>  - 8275731: CDS archived enums objects are recreated at runtime
>
> src/hotspot/share/cds/cdsHeapVerifier.cpp line 165:
> 
>> 163: 
>> 164:     ResourceMark rm;
>> 165:     for (JavaFieldStream fs(ik); !fs.done(); fs.next()) {
> 
> Can this call instead
> void InstanceKlass::do_local_static_fields(void f(fieldDescriptor*, Handle, TRAPS), Handle mirror, TRAPS) {
> and have this next few lines in the function?

I moved the code inside a new class CDSHeapVerifier::CheckStaticFields so I can call  InstanceKlass::do_local_static_fields

> src/hotspot/share/cds/cdsHeapVerifier.cpp line 254:
> 
>> 252:       InstanceKlass* ik = InstanceKlass::cast(k);
>> 253:       for (JavaFieldStream fs(ik); !fs.done(); fs.next()) {
>> 254:         if (!fs.access_flags().is_static()) {
> 
> same here.  It only saves a couple of lines but then you can have the function outside this large function.

You actually found a bug here. I am iterating over non-static fields and should walk the inherited fields as well. I changed the code to call InstanceKlass::do_nonstatic_fields()

> src/hotspot/share/cds/cdsHeapVerifier.hpp line 52:
> 
>> 50:       mtClassShared,
>> 51:       HeapShared::oop_hash> _table;
>> 52: 
> 
> Is this only used inside cdsHeapVerifier?  if so it should be in the .cpp file. There's also an ArchiveableStaticFieldInfo.  Not sure how they are related.

This `_table` is part of the CDSHeapVerifier instance, which is stack allocated. So I need to declare it as part of the CDSHeapVerifier class declaration in the hpp file.

> src/hotspot/share/cds/heapShared.cpp line 433:
> 
>> 431:   oop mirror = k->java_mirror();
>> 432:   int i = 0;
>> 433:   for (JavaFieldStream fs(k); !fs.done(); fs.next()) {
> 
> This seems like it should also use InstanceKlass::do_local_static_fields.

Converting this to InstanceKlass::do_nonstatic_fields() is difficult because the loop body references 7 different variables declared outside of the loop. 

One thing I tried is to add a new version of do_nonstatic_fields2() that supports C++ lambdas. You can see my experiment from here: 

https://github.com/openjdk/jdk/compare/master...iklam:lambda-for-instanceklass-do_local_static_fields2?expand=1

I changed all my new code to use the do_nonstatic_fields2() function with lambda.

> src/hotspot/share/cds/heapShared.cpp line 482:
> 
>> 480:     copy_open_objects(open_regions);
>> 481: 
>> 482:     CDSHeapVerifier::verify();
> 
> Should all this be DEBUG_ONLY ?

I changed CDSHeapVerifier::verify() to a NOT_DEBUG_RETURN function.

> src/hotspot/share/cds/heapShared.hpp line 236:
> 
>> 234:     oop _referrer;
>> 235:     oop _obj;
>> 236:     CachedOopInfo() :_subgraph_info(), _referrer(), _obj() {}
> 
> Should these be initialized to nullptr? does this do this?

These three fields are initialized with the default initializer (empty parenthesis) so they will be initialized to the null pointer.

-------------

PR: https://git.openjdk.java.net/jdk/pull/6653