Hi Aleksey,
On 11/11/2014 12:19 AM, Aleksey Shipilev wrote:
Hi David, Chris,
On 11/10/2014 04:53 PM, Chris Hegarty wrote:
On 10/11/14 12:56, David Holmes wrote:
On 10/11/2014 9:52 PM, Chris Hegarty wrote:
I have only looked at the libraries changes, and I think they make sense
. As in, I can find no reason why the name cannot be changed to be a
String.
Very quick response, but IIRC this has been examined in the past and
there were reasons why it can't/shouldn't be done. Will try to dig out
more details in the morning.
If there was previous discussion on this, that revealed some substantial
issue, that would be great, but I can't recall, or find, it now.
Hotspot express, and the desire for hotspot to run with different
library versions, would certainly cause complication, but I don't
believe that is an issue now.
Just on that, the library changes are minimal, and if this were to
proceed then they can accompany the hotspot change, as they make their
way into jdk9/dev.
Anyway, this should await your reply.
Alan was having the same concern, there is an issue with JNI/JVMTI and
other power users that might break when exposed to under-constructed
Thread, e.g:
https://bugs.openjdk.java.net/browse/JDK-6412693
This is why I ran jvmti and serviceability tests for this change,
yielding no failures. This reinforces my belief this patch does not
break the important invariant: if there is a problem with "Thread.name =
name.toCharArray()" anywhere in Thread code, then "Thread.name = name"
does neither regress it further nor fixes it.
True.
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).
Caching the name feels like a band-aid, that will probably complicate
the Thread initialization on VM side even more. Let's wait and see if
David can come up with some horror issue we are overlooking. :)
I don't see how a Java side cache affects anything on the VM
initialization side - and as Strings can be published unsafely we don't
even need sync/volatile to do so :)
That aside I think it is as Alan commented - a number of small things
(some logistical I think) that made this change not worth the effort.
Maybe now it is worth the effort if getName is a bottleneck (but again
caching is the common fix for that kind of problem :)). I was concerned
about executing even more Java code at thread attach time, but we
already create a String to pass to the Thread constructor, so no change
there.
So looking at your proposal ... some minor comments ...
JDK change is okay - but "name" doesn't need to be volatile when it is a
String reference.
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. :( )
---
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.
---
src/share/vm/prims/jvmtiTrace.cpp
Copyright year needs updating. :)
---
Aside: I wonder if we've inadvertently fixed 6771287 now. :) That was a
fun one to debug.
Thanks,
David
-----
Thanks,
-Aleksey.
