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

[David Holmes]

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