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.
