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.