RFR (S) 8059677: Thread.getName() instantiates Strings

[David Holmes]

On 11/11/2014 7:38 PM, Aleksey Shipilev wrote:
> Hi David,
>
> Updated webrevs will follow after I respin the tests. Meanwhile, some
> comments below:
>
> On 11.11.2014 10:06, David Holmes wrote:
>> On 11/11/2014 12:19 AM, Aleksey Shipilev wrote:
>>> Then I speculated that having char[] name would help VM initialize the
>>> name if we wanted to switch to complete VM-side initialization of
>>> Thread, but it seems we can do String oop instantiation in the similar
>>> vein.
>>
>> I think it really just came down to accessing the Thread name from
>> things like JVMDI/PI (now JVM TI) - easier for C code to access a raw
>> char[]. Maybe once upon a time (in a land not so far away) we even
>> passed char[] to the Thread constructor? :) But having re-discovered
>> past discussions etc there's really nothing to stop this from being a
>> String (slight memory use increase per Thread object).
>
> Yes. char[] does appear simpler from the native side, if not that pesky
> Unicode requirement that forces use to use Unicode routines within the
> VM to deal with char[] exposed to the Java side. Not so much an
> improvement comparing to String oop dance.
>
>
>> JDK change is okay - but "name" doesn't need to be volatile when it is a
>> String reference.
>
> I understand the memory model reasoning about the correctness, but I
> think users rightfully expect getName() to return the last "updated"
> Thread.name, even though this requirement is not spelled out
> specifically. Therefore, I believe "volatile" should stay.
>
> (I would be violently disappointed about the JDK if I realized my
> logging is garbled and the same thread "appears" under several names
> back and forth within a short time window -- because of data race on
> Thread.name)

Yes - silly of me. I was thinking the name is only set once but of 
course it can be set many times.

Cheers,
David
------


>> Hotspot side:
>>
>> src/share/vm/classfile/javaClasses.hpp
>>
>> This added assert seems overly cautious:
>>
>>   134     oop value = java_string->obj_field(value_offset);
>>   135     assert((value->is_typeArray() &&
>> TypeArrayKlass::cast(value->klass())->element_type() == T_CHAR), "expect
>> char[]");
>>
>> you are basically checking that String.value is defined as a char[]. If
>> warranted, this is a check needed once in the lifetime of a VM not every
>> time this method is called. (Yes I see we had something similarly odd in
>> java_lang_thread::name. :( )
>
> Agreed. Dropped the assert from here. I think we already check this for
> String.name field when we pre-compute the value_offset.
>
>
>> ---
>>
>> src/share/vm/classfile/javaClasses.cpp
>>
>> ! oop java_lang_Thread::name(oop java_thread) {
>>      oop name = java_thread->obj_field(_name_offset);
>> !   assert(name != NULL, "thread name is NULL");
>>
>> I'm not confident this can never be called before the name has been set.
>> The original assertion allowed for NULL as does the JVM TI code.
>
> Agreed. Dropped the assert altogether.
>
>
>> ---
>>
>> src/share/vm/prims/jvmtiTrace.cpp
>>
>> Copyright year needs updating. :)
>
> Done.
>
>
>> ---
>>
>> Aside: I wonder if we've inadvertently fixed 6771287 now. :) That was a
>> fun one to debug.
>
> Ouch.
>
>
> -Aleksey.
>
>
>