Hi Andrew,
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 ) \
?
---
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. ??
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.)
---
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...
---
src/java.base/share/classes/jdk/internal/misc/UnsafeConstants.java
31 * package-protected ...
s/protected/private/
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.
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.
85 * flag whose value ...
92 * flag whose value ...
s/flag/Flag/
Thanks,
David
On 29/03/2019 2:56 am, Andrew Dinn wrote:
On 28/03/2019 15:22, Thomas Stüfe wrote:
The second of those was actually intended to be iff. This is a common
abbreviation used by English/US mathematicians and logicians to write
'if and only if' (it is also sometimes written as <=>). Since you didn't
recognize it I guess I really need to write it out in full.
Oh, don't worry on my account. I am not a native speaker nor a
mathematician. You could leave iff and add [sic] to make everyone
curious and start googling "iff" :)
I changed it nevertheless. It would be remiss to presume readers need be
conversant with elliptical, English logico-mathematical parlance (I'm
explaining this in as plain English as I can ... no, wait! ;-).
Anyway, I addressed this and your other concerns in a new webrev.
JIRA: https://bugs.openjdk.java.net/browse/JDK-8221477
Webrev: http://cr.openjdk.java.net/~adinn/8221477/webrev.02
Input from a second reviewer would be welcome ...
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
