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
