RFR: 8221477: Inject os/cpu-specific constants into Unsafe from JVM

[David Holmes]

Follow up ...

On 1/04/2019 2:27 pm, David Holmes wrote:
> Hi Andrew,
> 
> On 29/03/2019 8:40 pm, Andrew Dinn wrote:
>> Hi David,
>>
>> Thanks very much for reviewing this patch.
>>
>> On 29/03/2019 01:25, David Holmes wrote:
>>> This seems fine in general but I have a few queries on some details:
>>>
>>> src/hotspot/share/classfile/javaClasses.hpp
>>>
>>>      f(java_lang_Thread) \
>>> +   f(jdk_internal_misc_UnsafeConstants) \
>>>      f(java_lang_ThreadGroup) \
>>>
>>> Is there a reason this needs to be shoved in there? Similarly with
>>> src/hotspot/share/classfile/systemDictionary.hpp:
>>>
>>>     do_klass(Thread_klass,
>>> java_lang_Thread                                      ) \
>>> +   do_klass(UnsafeConstants_klass,
>>> jdk_internal_misc_UnsafeConstants                     ) \
>>>      do_klass(ThreadGroup_klass,
>>> java_lang_ThreadGroup                                 ) \
>>>
>>> ?
>>
>> I'm not sure what you are asking here. Are you talking about the
>> positioning of these entries? If so then the reason for choosing those
>> positions was because they match the position of the corresponding class
>> initialization calls in the file thread.cpp. As to that choice ... see
>> below.
> 
> Yes I'm asking about the positioning ...
> 
>>> src/hotspot/share/runtime/thread.cpp
>>>
>>>      main_thread->set_threadObj(thread_object);
>>> +
>>> +   // initialize the hardware-specific constants needed by Unsafe
>>> +   initialize_class(vmSymbols::jdk_internal_misc_UnsafeConstants(),
>>> CHECK);
>>> +   jdk_internal_misc_UnsafeConstants::set_unsafe_constants();
>>> +
>>>      // Set thread status to running since main thread has
>>>      // been started and running.
>>>      java_lang_Thread::set_thread_status(thread_object,
>>>
>>> That seems a very odd place to insert the new initialization. I can't
>>> see any reason you'd need to split the Thread object initialization like
>>> that. ??
>>
>> Well, yes, indeed :-). I was not sure where this init needed to go so I
>> simply put it in as early as I thought was safe. Clearly, it is an
>> interloper into intitialise_java_lang_classes() but I was not sure where
>> else was more appropriate.
>>
>> I don't think it matters too much where this init happens so long as it
>> is early enough to precede any clinit dependencies on Unsafe (see
>> below). I'm very happy to take advice on this (indeed I was hoping/
>> expecting it would be brought up at review) and also on where the
>> entries in the headers would best be placed.
>>
>>> More generally exactly when do you need to initialize this new class by
>>> and how does the initialization order change before/after this fix? (I'm
>>> expecting only to see the new class inserted where needed without any
>>> other perturbations.)
>>
>> As I said, I put the class initialization in at this early point simply
>> to guarantee that the constants are available before Unsafe or any class
>> which might recursively initialize Unsafe could end up needing them. I
>> am sure they could move later and also earlier, although the latter
>> would probably not make any sense. The important thing is that they
>> don't really have any hard dependencies on other class inits apart,
>> perhaps, from 'magic' classes like Object, Class etc which need to exist
>> in order to init /any/ other class.
> 
> I did some analysis of the class loading and initialization sequence and 
> added my suggestions to bug report. In summary loading seems somewhat 
> immaterial so I suggest:
> 
> - javaClasses.hpp: Add UnsafeConstants at the end of 
> BASIC_JAVA_CLASSES_DO_PART2
> - systemDictionary.hpp: Add UnsafeConstants immediately before Unsafe
> 
> Then for init:
> - thread.cpp: initialize UnsafeConstants immediately after j.l.Module
> 
> It would be desirable to detect if we happen to execute the <clinit> 
> earlier (by accident) so I suggest adding a "not initialized" assertion 
> prior to your code calling initialize_class. Actually that might be a 
> useful addition to the initialize_class method, as if it fires it means 
> we're not initializing in the expected order ... I'll run a little 
> adding that ...

I meant to say "run a little test". Turns out you can't put the 
assertion in initialize_class as the initialization can vary for some of 
the exception classes (at least) depending on VM flags used (e.g. -Xrs, 
and I think certain logging options).

Thanks,
David

> 
> Thanks,
> David
> 
> P.S. This missing space in javaClasses.cpp was reported by Thomas but 
> hasn't been fixed yet:
> 
> 4026     }else {
> 
> 
>> I deliberately factored these constants out of Unsafe into a separate
>> all static class UnsafeConstants so as to decouple this init from any
>> current or future dependencies on Unsafe (and also to make them more
>> clearly visible). Since the fields only hold os/hw specific constants
>> they can be set any time after VM_Version/CPU-specific init and
>> OS-specific init has completed.
>>
>>> src/hotspot/share/classfile/vmSymbols.hpp
>>>
>>> +   template(big_endian_name,                           "BIG_ENDIAN")
>>>                             \
>>> +   template(use_unaligned_access_name,
>>> "UNALIGNED_ACCESS")                        \
>>>
>>> Nit: There's an extra space before "UNALIGNED...
>>
>> Thanks. Thomas Stuefe already spotted that and I have updated the webrev
>> in place to fix it.
>>
>>> src/java.base/share/classes/jdk/internal/misc/UnsafeConstants.java
>>>
>>> 31  * package-protected  ...
>>>
>>> s/protected/private/
>>
>> Thanks, will correct it.
>>
>>>   37  * The JVM injects values into all the static fields of this class
>>>   ...
>>>   43  * hardware-specific constants installed by the JVM.
>>>
>>> I get this gist of this note but still found it rather difficult to
>>> intepret. There are I think two key points:
>>>
>>> 1. The fields must not be constant variables, hence the need for the
>>> static initialization block.
>>> 2. The true value of each field is injected by the VM.
>>>
>>> How about:
>>>
>>> * The JVM injects hardware-specific values into all the static fields
>>> * of this class during JVM initialization. The static initialization
>>> * block exists to prevent the fields from being considered constant
>>> * variables, so the field values will be not be compiled directly into
>>> * any class that uses them.
>>
>> Yes, I agree that is clearer.
>>
>>> 60      * @implNote
>>> 61      * The actual value for this field is injected by JVM. A static
>>> 62      * initialization block is used to set the value here to
>>> 63      * communicate that this static final field is not statically
>>> 64      * foldable, and to avoid any possible circular dependency during
>>> 65      * vm initialization.
>>>
>>> I think all you need for each field is just:
>>>
>>> * @implNote
>>> * The actual value for this field is injected by _the_ JVM.
>>
>> Yes, also better.
>>
>>> 85      * flag whose value ...
>>> 92      * flag whose value ...
>>>
>>> s/flag/Flag/
>>
>> Thanks, will correct this.
>>
>> regards,
>>
>>
>> Andrew Dinn
>> -----------
>> Senior Principal Software Engineer
>> Red Hat UK Ltd
>> Registered in England and Wales under Company Registration No. 03798903
>> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
>>