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

[David Holmes]

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

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
>