RFR: 8293979: Resolve JVM_CONSTANT_Class references at CDS dump time [v2]

[Ioi Lam]

On Mon, 17 Oct 2022 23:02:00 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 three additional commits since the last revision:
>> 
>>  - @coleenp comments
>>  - Merge branch 'master' into 8293979-resolve-class-references-at-dumptime
>>  - 8293979: Resolve JVM_CONSTANT_Class references at CDS dump time
>
> src/hotspot/share/cds/classPrelinker.cpp line 48:
> 
>> 46:     InstanceKlass* super = ik->java_super();
>> 47:     if (super != NULL) {
>> 48:       add_one_vm_class(super);
> 
> Do you care about order? Would it be better to add the superclasses before the instanceKlass?

The order doesn't matter since this is a hashtable. Also, I am trying to avoid walking up the hierarchy if a class is already in the table.

> src/hotspot/share/cds/classPrelinker.cpp line 59:
> 
>> 57: ClassPrelinker::ClassPrelinker() {
>> 58:   assert(_singleton == NULL, "must be");
>> 59:   _singleton = this;
> 
> I'm not sure why you have this?

I was trying to put the states of the ClassPrelinker in its instance fields, but this is probably confusing. I'll change ClassPrelinker to AllStatic instead.

> src/hotspot/share/cds/classPrelinker.cpp line 116:
> 
>> 114: Klass* ClassPrelinker::get_resolved_klass_or_null(ConstantPool* cp, int cp_index) {
>> 115:   if (cp->tag_at(cp_index).is_klass()) {
>> 116:     CPKlassSlot kslot = cp->klass_slot_at(cp_index);
> 
> This is another place that CPKlassSlot leaks out.  It would be nice if these were in constantPool.  Maybe this function belongs in the constant pool.

Changed to use ConstantPool::resolved_klass_at() instead.

> src/hotspot/share/cds/classPrelinker.cpp line 127:
> 
>> 125: }
>> 126: 
>> 127: bool ClassPrelinker::can_archive_resolved_klass(ConstantPool* cp, int cp_index) {
> 
> Can the other can_archive_resolved_klass be just above this one so we know why their names are the same?  Also can_archive_vm_resolved_klass is too closely named.

I moved the two `can_archive_resolved_klass()` functions next to each other. I also folded `can_archive_vm_resolved_klass` into `can_archive_resolved_klass(InstanceKlass*, Klass*)`, simplified it and improved the comments.

> src/hotspot/share/cds/classPrelinker.cpp line 146:
> 
>> 144: 
>> 145:   bool first_time;
>> 146:   _processed_classes.put_if_absent(ik, &first_time);
> 
> I don't see any get functions for this hashtable?

This table is used to check if we have already worked on the class:


  bool first_time;
  _processed_classes.put_if_absent(ik, &first_time);
  if (!first_time) {
    return;
  }

> src/hotspot/share/cds/classPrelinker.cpp line 194:
> 
>> 192:   CPKlassSlot kslot = cp->klass_slot_at(cp_index);
>> 193:   int name_index = kslot.name_index();
>> 194:   Symbol* name = cp->symbol_at(name_index);
> 
> Isn't this klass_name_at() ?  I think these details should stay in constantPool.cpp.

I changed it to use klass_name_at().

> src/hotspot/share/cds/classPrelinker.cpp line 205:
> 
>> 203: 
>> 204: #if INCLUDE_CDS_JAVA_HEAP
>> 205: void ClassPrelinker::resolve_string(constantPoolHandle cp, int cp_index, TRAPS) {
> 
> Is this function only needed in this cpp file? Can you make it static and defined before its caller? Are there other functions where this is true?  Does it need to be a member function of ClassPrelinker?

I prefer to make such methods private, so it can access other private members of the same class.

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

PR: https://git.openjdk.org/jdk/pull/10330