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

[coleen.phillimore at oracle.com]

http://cr.openjdk.java.net/~shade/8216302/webrev.02/src/hotspot/share/classfile/javaClasses.cpp.udiff.html

+ oop class_oop = holder->java_mirror();


This is a naked oop.  You should use Handle and HandleMark in this function.

1347 oop java_lang_Class::name(oop java_class, TRAPS) {


So is this.  You should pass a Handle to this.

Otherwise, this looks really good.

Thanks,
Coleen

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