RFR (S) 8216302: StackTraceElement::fill_in can use cached Class.name

[coleen.phillimore at oracle.com]

On 1/8/19 4:56 AM, Aleksey Shipilev wrote:
> On 1/8/19 3:15 AM, David Holmes wrote:
>> It seems somewhat awkward to me to have two different code paths for initializing the
>> java.lang.Class name field. Can this be restructured a little more (change Class.getName()) so that
>> the VM always initializes "name" and then JVM_GetClassName could just call java_lang_Class::name,
>> rather than duplicating the logic?
> Mmm. I am afraid to do this eagerly because of more memory footprint and potential bootstrapping
> issues. Also, I want to keep this open to implement a crazy footprint-reducing idea: nulling the
> fields like Class.name to conserve footprint at expense of additional call to reinstate the value
> afterwards.

I agree we shouldn't initialize the name field eagerly when creating a 
class.  I also looked at this code path:

     public String getName() {
         String name = this.name;
         if (name == null)
             this.name = name = getName0();
         return name;
     }

It looks like when we call JVM_GetClassName, we're initializing the 
Class.name field by the caller.

Maybe could rewrite the java/lang/Class version to be:

     public String getName() {
         String name = this.name;
         if (name == null)
             name = getName0();  // this initializes this.name yuck.
         return name;
     }

and have JVM_GetClassName call java_lang_Class::name() to do the 
initialization.   Seems not worth it just to avoid duplicating these 
lines in both java_lang_Class::name() and JVM_GetClassName.

+ const char* name = 
java_lang_Class::as_external_name(JNIHandles::resolve(cls));
+ oop result = StringTable::intern((char*)name, CHECK_NULL);

Coleen

>> Specific comments:
>>
>> src/hotspot/share/classfile/javaClasses.cpp
>>
>> !     Klass* k = as_Klass(java_class);
>> !     assert(k->is_klass(), "just checking");
>> !     name = k->external_name();
>>
>> as_Klass already has the requisite assertions so there was no reason to change this part of the
>> code. I see that jvm.cpp already contains the same redundant logic.
> Right. Ditched that change.
>
>> Copyright years need updating.
> Updated.
>
> Also made the test more up-to-the-point, with clearing the Class.name cache explicitly.
>
> New webrev:
>    http://cr.openjdk.java.net/~shade/8216302/webrev.02/
>
> Thanks,
> -Aleksey
>