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

[Aleksey Shipilev]

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)

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