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