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

[David Holmes]

On 9/01/2019 1:51 am, Aleksey Shipilev wrote:
> On 1/8/19 12:27 PM, David Holmes wrote:
>> 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)
>>    getName0(); // calls JVM_GetClassName
>> return this.name;
>>
>> it just does:
>>
>> if (this.name == null)
>>    getName0(); // calls JVM_GetClassName
>> return this.name;
> 
> Oh. I had this option on the table when doing the patch, but I disregarded it as dirty hack, because
> calling the getter for side effects *only* is awkward. Handling concurrency on Java side also looks
> simpler. There is a benign race on Class.name, and to remain benign, it should _not_ do the second
> read. In other word, null-checking this.name, and then returning the second read of this.name is not
> entirely correct.
> 
> The original code is checking and returning the local, not the second heap read:
> 
>      public String getName() {
>          String name = this.name;
>          if (name == null)
>              this.name = name = getName0();
>          return name;
>      }
> 
> This, I think, leaves us with taking the return value from getName0.

Sure introduce a local as needed.
> 
> If we wanted to handle JVM_GetClassName better, we could consider calling java_lang_Class::name from
> JVM_GetClassName, thus going through the cached path. That would do the two stores, though: one
> through javaClasses, and another on Java side. Seeing how there is only a single use of
> JVM_GetClassName, and that use is already-cached Class.getName0, I see no reason to do the excessive
> write.

What I don't like is the fact that there is an illusion that the name 
field is only set by getName() when in fact it can also be set by the 
VM. So you already have a very hidden side-effect. If JVM_GetClassName 
is used to call java_lang_class::Name and set the name field then at 
least you can document the side-effect.

David

> -Aleksey
>