On Mon, 17 Jan 2022 18:36:35 GMT, Coleen Phillimore <cole...@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
