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

[David Holmes]

On 8/01/2019 7:56 pm, 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. 

I said nothing about doing this eagerly. All I'm suggesting is that 
instead of getName() doing:

if (this.name == null)
   this.name = getName0(); // calls JVM_GetClassName
return this.name;

it just does:

if (this.name == null)
   getName0(); // calls JVM_GetClassName
return this.name;

and JVM_GetClassName calls java_lang_Class::name() which sets 
"this.name" as a side-effect (as it does if called from the stacktrace 
code).

> 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 don't see any impact on that plan. Of course you'll need to ensure the 
cached name is updated in a thread-safe manner.

Cheers,
David
-----

>> 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
>