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

[Peter Levart]

On 4/5/19 10:48 AM, Andrew Dinn wrote:

Hi Peter,

Regarding the static initializer ... there is an explanatory implNote
explaining the rationale for the static block in the class javadoc at
the top of the file. I agree this could be improved by explaining that
the block is executed and then its settings are overridden:

  * @implNote
  *
  * The JVM injects hardware-specific values into all the static fields
  * of this class during JVM initialization. The static initialization
  * block is executed when the class is initialized then JVM injection
  * updates the fields with the correct constants. The static block
  * is required 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.

Regarding the field Javadoc ... I understand that an OpenJDK dev might
want a correct and complete model for what exactly happens during init
however that is rather a moot point as regards semantics of the value in
the Java code. The nett effect is as the javadoc states -- the value is
injected by the JVM and, per the text above, that value identifies the
relevant hardware/os config parameter. So, I'll stop at expanding the
class-level comment.


Just for the peace of mind of casual readers or perhaps someone that
might later add a field to this class and try to initialize it in the
static initializer, although I think this class is reserved for injected
fields only...

Understood. I think the class-level comment already makes that latter
detail explicit and the revised version gives enough warning to devs.

I hope the above changes is acceptable.


It is precisely what I was thinking about. It's understood that the 
@implNote on fields is enough for the users of the UnsafeConstants class 
since the injection happens magically as "part of class initialization" 
for them. The only place where the timing of injection is observable 
from Java code is in the class initializer itself where the fields are 
set to default values and injection hasn't happened yet...


Regards, Peter


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

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

[Thomas Stüfe]

Still looking good :)

..Thomas

On Thu, Apr 4, 2019 at 5:19 PM Andrew Dinn  wrote:

> New webrev is now available. Re-reviews welcome.
>
> JIRA:   https://bugs.openjdk.java.net/browse/JDK-8221477
> Webrev: http://cr.openjdk.java.net/~adinn/8221477/webrev.03
>
> This version should address all comments from Thomas, David and Coleen.
>
> Testing
> Tier1 test passed.
> Submit test passed.
>
> 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
>

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

[Andrew Dinn]

HI David,

On 05/04/2019 04:54, David Holmes wrote:
> Hi Andrew,
> 
> This all looks good to me - thanks for making the changes.

Thank you for the corrections and improvements.

> Two nits:
> - BE was barely acceptable when used like a local variable, but now I
> think BIG_ENDIAN would be better.
> - If you don't use static import you can name the UnsafeConstants fields
> the obvious way without clashing with public fields in Unsafe.
> 
> Feel free to ignore those nits. If you do make a change no need to see
> another webrev.
Well, I changed BE to BIG_ENDIAN and it doesn't look as messy as I
thought. So BIG_ENDIAN it is.

However, omitting the static import requires UnsafeConstants.BIG_ENDIAN
at multiple mentions. That does look too cluttered for comfortable reading.

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

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

[Andrew Dinn]

Hi Peter,

Thanks for the last-minute recommendation!

On 05/04/2019 08:37, Peter Levart wrote:
> For a casual reader (like me) the comments in the UnsafeConstants for
> each field:
> 
>   69  * @implNote
>   70  * The actual value for this field is injected by the JVM.
> 
> ...make one wonder what is actually going on regarding the static
> initializer at the end of the class. Is it actually executed? Is it
> there just to silence the javac and prevent fields from becoming
> compile-time constants?

Regarding the static initializer ... there is an explanatory implNote
explaining the rationale for the static block in the class javadoc at
the top of the file. I agree this could be improved by explaining that
the block is executed and then its settings are overridden:

 * @implNote
 *
 * The JVM injects hardware-specific values into all the static fields
 * of this class during JVM initialization. The static initialization
 * block is executed when the class is initialized then JVM injection
 * updates the fields with the correct constants. The static block
 * is required 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.

Regarding the field Javadoc ... I understand that an OpenJDK dev might
want a correct and complete model for what exactly happens during init
however that is rather a moot point as regards semantics of the value in
the Java code. The nett effect is as the javadoc states -- the value is
injected by the JVM and, per the text above, that value identifies the
relevant hardware/os config parameter. So, I'll stop at expanding the
class-level comment.

> Just for the peace of mind of casual readers or perhaps someone that
> might later add a field to this class and try to initialize it in the
> static initializer, although I think this class is reserved for injected
> fields only...

Understood. I think the class-level comment already makes that latter
detail explicit and the revised version gives enough warning to devs.

I hope the above changes is acceptable.

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

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

[David Holmes]

Hi Peter,

On 5/04/2019 5:37 pm, Peter Levart wrote:

Hi Andrew,

For a casual reader (like me) the comments in the UnsafeConstants for 
each field:


   69  * @implNote
   70  * The actual value for this field is injected by the JVM.

...make one wonder what is actually going on regarding the static 
initializer at the end of the class. Is it actually executed? Is it 
there just to silence the javac and prevent fields from becoming 
compile-time constants?


That's what the earlier class comment is about:

 * @implNote
 *
 * 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.
 */

Cheers,
David


Reading the patch further uncovers the secret:

3645   // initialize the hardware-specific constants needed by Unsafe
3646 initialize_class(vmSymbols::jdk_internal_misc_UnsafeConstants(), 
CHECK);

3647   jdk_internal_misc_UnsafeConstants::set_unsafe_constants();

If my understanding is correct, then the static initializer *is* 
executed and then the fields' values are overwritten with injected values.


So perhaps it might be nice to write that down in the javadoc like:

   69  * @implNote
   70  * The actual value for this field is injected by the JVM 
immediately after the class initialization.


Or something similar in the class-level javadoc.

Just for the peace of mind of casual readers or perhaps someone that 
might later add a field to this class and try to initialize it in the 
static initializer, although I think this class is reserved for injected 
fields only...


Regards, Peter

On 4/4/19 5:19 PM, Andrew Dinn wrote:

New webrev is now available. Re-reviews welcome.

JIRA:   https://bugs.openjdk.java.net/browse/JDK-8221477
Webrev: http://cr.openjdk.java.net/~adinn/8221477/webrev.03

This version should address all comments from Thomas, David and Coleen.

Testing
Tier1 test passed.
Submit test passed.

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

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

[Peter Levart]

Hi Andrew,

For a casual reader (like me) the comments in the UnsafeConstants for 
each field:


  69  * @implNote
  70  * The actual value for this field is injected by the JVM.

...make one wonder what is actually going on regarding the static 
initializer at the end of the class. Is it actually executed? Is it 
there just to silence the javac and prevent fields from becoming 
compile-time constants?


Reading the patch further uncovers the secret:

3645   // initialize the hardware-specific constants needed by Unsafe
3646 initialize_class(vmSymbols::jdk_internal_misc_UnsafeConstants(), 
CHECK);

3647   jdk_internal_misc_UnsafeConstants::set_unsafe_constants();

If my understanding is correct, then the static initializer *is* 
executed and then the fields' values are overwritten with injected values.


So perhaps it might be nice to write that down in the javadoc like:

  69  * @implNote
  70  * The actual value for this field is injected by the JVM 
immediately after the class initialization.


Or something similar in the class-level javadoc.

Just for the peace of mind of casual readers or perhaps someone that 
might later add a field to this class and try to initialize it in the 
static initializer, although I think this class is reserved for injected 
fields only...


Regards, Peter

On 4/4/19 5:19 PM, Andrew Dinn wrote:

New webrev is now available. Re-reviews welcome.

JIRA:   https://bugs.openjdk.java.net/browse/JDK-8221477
Webrev: http://cr.openjdk.java.net/~adinn/8221477/webrev.03

This version should address all comments from Thomas, David and Coleen.

Testing
Tier1 test passed.
Submit test passed.

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

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

[David Holmes]

Hi Andrew,

This all looks good to me - thanks for making the changes.

Two nits:
- BE was barely acceptable when used like a local variable, but now I 
think BIG_ENDIAN would be better.
- If you don't use static import you can name the UnsafeConstants fields 
the obvious way without clashing with public fields in Unsafe.


Feel free to ignore those nits. If you do make a change no need to see 
another webrev.


Thanks,
David

On 5/04/2019 1:19 am, Andrew Dinn wrote:

New webrev is now available. Re-reviews welcome.

JIRA:   https://bugs.openjdk.java.net/browse/JDK-8221477
Webrev: http://cr.openjdk.java.net/~adinn/8221477/webrev.03

This version should address all comments from Thomas, David and Coleen.

Testing
Tier1 test passed.
Submit test passed.

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

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

[Andrew Dinn]

New webrev is now available. Re-reviews welcome.

JIRA:   https://bugs.openjdk.java.net/browse/JDK-8221477
Webrev: http://cr.openjdk.java.net/~adinn/8221477/webrev.03

This version should address all comments from Thomas, David and Coleen.

Testing
Tier1 test passed.
Submit test passed.

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

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

[Andrew Dinn]

Hi David,

Thanks very much for investigating this and posting your recommendations.

On 01/04/2019 08:15, David Holmes wrote:
>> 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

I have done all of the above. Also, in javaClasses.hpp to keep things
uniform I moved the class decl for jdk_internal_misc_UnsafeConstants to
follow that of class java_..._AbstractOwnableSynchronizer i.e. the class
decl order matches the order of the do_klass statements.

>> It would be desirable to detect if we happen to execute the 
>> 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).
Ok, thanks. I have added the following immediately before calling
initialize_class.

#ifdef ASSERT
  InstanceKlass *k = SystemDictionary::UnsafeConstants_klass();
  assert(k->is_not_initialized(), "UnsafeConstants should not already be
initialized");
#endif
  . . .

I'll wait for feedback from Coleen before sending an updated webrev.

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

Re: 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  
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

Re: 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  
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

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

[Andrew Dinn]

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 du

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

[Thomas Stüfe]

Thanks Andrew. Looks all good to me now.

Cheers, Thomas

On Fri, Mar 29, 2019 at 9:43 AM Andrew Dinn  wrote:

> On 28/03/2019 17:09, Thomas Stüfe wrote:
> > src/hotspot/share/classfile/javaClasses.cpp
> >
> > Pure nitpicking, but you could remove the member variables too and just
> > set the Unsafe members directly, e.g.:
> >
> > if (fd->name() == vmSymbols::address_size_name()) {
> >   mirror->int_field_put(fd->offset(), (jint)sizeof(void*));
> > } else if (fd->name() == vmSymbols::page_size_name()) {
> >   mirror->int_field_put(fd->offset(), (jint) os::vm_page_size());
> > ..
> >
> > and so on. But I leave this up to you. This is already better than
> before.
>
> I thought about that but it seemed to me to be better for clarity and
> readability to init all the members in a single block in the constructor
> rather than spread the computations of the field values through the code
> that updates the fields. I think it's easier that way to see where all
> the values are coming from and align them up with the fields in the
> target class.
>
> > +}else {
> > space missing before else
> Yes.
>
> > src/hotspot/share/classfile/vmSymbols.hpp
> > "UNALIGNED_ACCESS" off by one space
>
> Yes.
>
> > src/java.base/share/classes/java/nio/channels/FileChannel.java
> >
> > This may have creeped in from another change.
>
> Ah, yes. That was the next patch that I forgot to back out. I have
> updated the webrev in place to remove it and also fix the whitespace &
> alignment issues.
>
> > Good otherwise.
> Thanks.
>
> 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
>

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

[Andrew Dinn]

On 28/03/2019 17:09, Thomas Stüfe wrote:
> src/hotspot/share/classfile/javaClasses.cpp
> 
> Pure nitpicking, but you could remove the member variables too and just
> set the Unsafe members directly, e.g.:
> 
>     if (fd->name() == vmSymbols::address_size_name()) {
>       mirror->int_field_put(fd->offset(), (jint)sizeof(void*));
>     } else if (fd->name() == vmSymbols::page_size_name()) {
>       mirror->int_field_put(fd->offset(), (jint) os::vm_page_size());
> ..
> 
> and so on. But I leave this up to you. This is already better than before.

I thought about that but it seemed to me to be better for clarity and
readability to init all the members in a single block in the constructor
rather than spread the computations of the field values through the code
that updates the fields. I think it's easier that way to see where all
the values are coming from and align them up with the fields in the
target class.

> +    }else {
> space missing before else
Yes.

> src/hotspot/share/classfile/vmSymbols.hpp
> "UNALIGNED_ACCESS" off by one space

Yes.

> src/java.base/share/classes/java/nio/channels/FileChannel.java
> 
> This may have creeped in from another change.

Ah, yes. That was the next patch that I forgot to back out. I have
updated the webrev in place to remove it and also fix the whitespace &
alignment issues.

> Good otherwise.
Thanks.

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

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

[David Holmes]

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

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

[Thomas Stüfe]

On Thu, Mar 28, 2019 at 5:56 PM 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
>
>
src/hotspot/share/classfile/javaClasses.cpp

Pure nitpicking, but you could remove the member variables too and just set
the Unsafe members directly, e.g.:

if (fd->name() == vmSymbols::address_size_name()) {
  mirror->int_field_put(fd->offset(), (jint)sizeof(void*));
} else if (fd->name() == vmSymbols::page_size_name()) {
  mirror->int_field_put(fd->offset(), (jint) os::vm_page_size());
..

and so on. But I leave this up to you. This is already better than before.

+}else {
space missing before else

--

src/hotspot/share/classfile/vmSymbols.hpp
"UNALIGNED_ACCESS" off by one space

---

src/java.base/share/classes/java/nio/channels/FileChannel.java

This may have creeped in from another change.

---

Good otherwise.

Cheers, Thomas



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

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

[Andrew Dinn]

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

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

[Andrew Haley]

On 3/28/19 3:22 PM, Thomas Stüfe wrote:
> On Thu, Mar 28, 2019 at 3:41 PM Andrew Dinn  wrote:
>
>> s/iff/if
>> 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" :)

Dijkstra:

  The notation iff is used for "if and only if". A few years ago,
  while lecturing in Denmark, I used Fif instead, reasoning that since
  "if and only if" was a symmetric concept its notation should be
  symmetric also.  Without knowing it, I had punned in Danish and the
  audience laughed, for fif in Danish means "a little trick". I
  resolved thereafter to use fif so I could tell my joke, but my
  colleagues talked me out of it.

:-)

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. 
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

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

[Andrew Haley]

On 3/28/19 4:23 PM, Andrew Haley wrote:
> Dijkstra:
> 
>   The notation iff is used for "if and only if". A few years ago,
>   while lecturing in Denmark, I used Fif instead, reasoning that since
>   "if and only if" was a symmetric concept its notation should be
>   symmetric also.  Without knowing it, I had punned in Danish and the
>   audience laughed, for fif in Danish means "a little trick". I
>   resolved thereafter to use fif so I could tell my joke, but my
>   colleagues talked me out of it.

Arggh! Sorry, that was Gries, not Dijkstra. I misremembered.

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. 
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

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

[Thomas Stüfe]

Btw congrats for finally getting JEP 352 to move on. That really took a
while.

Cheers, Thomas

On Thu, Mar 28, 2019 at 4:22 PM Thomas Stüfe 
wrote:

>
>
> On Thu, Mar 28, 2019 at 3:41 PM Andrew Dinn  wrote:
>
>> On 28/03/2019 12:17, Thomas Stüfe wrote:
>> > this looks fine, nits only:
>>
>> Thank you for the review, Thomas. I'll post a follow up webrev to
>> address the comments. Responses are inline.
>>
>> > UnsafeConstantsFixup::do_field()
>> >
>> > you could shrink that coding a bit by factoring out the checks, e.g.:
>> > . . .
>>
>> Yes, that was egregious cut-and-paste coding.
>>
>> Actually, the idea is that all fields of the class are final static and
>> all of them are expected to be injected (which is why the final else
>> clause asserts).
>>
>> So, I could just move the asserts up to precede the if-elseif cascade
>> and also include checks that the field is final and static.
>>
>>   void do_field(fieldDescriptor* fd) {
>> oop mirror = fd->field_holder()->java_mirror();
>> assert(mirror != NULL, "UnsafeConstants must have mirror already");
>> assert(fd->field_holder() ==
>> SystemDictionary::UnsafeConstants_klass(), "Should be UnsafeConstants");
>> assert(fd->is_final(), "fields of UnsafeConstants must be final");
>> assert(fd->is_static() "fields of UnsafeConstants must be static");
>> if (fd->name() == vmSymbols::address_size_name()) {
>>   mirror->int_field_put(fd->offset(), _address_size);
>> } else if (fd->name() == vmSymbols::page_size_name()) {
>>   mirror->int_field_put(fd->offset(), _page_size);
>> } else if (fd->name() == vmSymbols::big_endian_name()) {
>>   . . .
>> } else {
>>   assert(false, "unexpected UnsafeConstants field");
>> }
>>
>> > Also, is there a reason that code from thread.cpp:
>> >
>> > jint address_size = sizeof(void*);
>> > jint page_size = os::vm_page_size();
>> > jboolean is_big_endian = LITTLE_ENDIAN_ONLY(false)
>> > BIG_ENDIAN_ONLY(true);
>> > jint use_unaligned_access = UseUnalignedAccesses;
>> >
>> > could not just happen right in UnsafeConstantsFixup::do_field()? You
>> > would not have to pass all these arguments around, param list could be
>> > empty.
>>
>> No good reason. Your suggestion is better.
>>
>> > src/hotspot/share/classfile/vmSymbols.hpp
>> >
>> > Alignment of trailing '\' broken
>>
>> Oops!
>>
>> > src/java.base/share/classes/jdk/internal/misc/UnsafeConstants.java
>> >
>> > s/intitialize/initialize
>> > s/iff/if
>> 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" :)
>
> ..Thomas
>
>
>>
>> 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
>>
>

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

[Thomas Stüfe]

On Thu, Mar 28, 2019 at 3:41 PM Andrew Dinn  wrote:

> On 28/03/2019 12:17, Thomas Stüfe wrote:
> > this looks fine, nits only:
>
> Thank you for the review, Thomas. I'll post a follow up webrev to
> address the comments. Responses are inline.
>
> > UnsafeConstantsFixup::do_field()
> >
> > you could shrink that coding a bit by factoring out the checks, e.g.:
> > . . .
>
> Yes, that was egregious cut-and-paste coding.
>
> Actually, the idea is that all fields of the class are final static and
> all of them are expected to be injected (which is why the final else
> clause asserts).
>
> So, I could just move the asserts up to precede the if-elseif cascade
> and also include checks that the field is final and static.
>
>   void do_field(fieldDescriptor* fd) {
> oop mirror = fd->field_holder()->java_mirror();
> assert(mirror != NULL, "UnsafeConstants must have mirror already");
> assert(fd->field_holder() ==
> SystemDictionary::UnsafeConstants_klass(), "Should be UnsafeConstants");
> assert(fd->is_final(), "fields of UnsafeConstants must be final");
> assert(fd->is_static() "fields of UnsafeConstants must be static");
> if (fd->name() == vmSymbols::address_size_name()) {
>   mirror->int_field_put(fd->offset(), _address_size);
> } else if (fd->name() == vmSymbols::page_size_name()) {
>   mirror->int_field_put(fd->offset(), _page_size);
> } else if (fd->name() == vmSymbols::big_endian_name()) {
>   . . .
> } else {
>   assert(false, "unexpected UnsafeConstants field");
> }
>
> > Also, is there a reason that code from thread.cpp:
> >
> > jint address_size = sizeof(void*);
> > jint page_size = os::vm_page_size();
> > jboolean is_big_endian = LITTLE_ENDIAN_ONLY(false)
> > BIG_ENDIAN_ONLY(true);
> > jint use_unaligned_access = UseUnalignedAccesses;
> >
> > could not just happen right in UnsafeConstantsFixup::do_field()? You
> > would not have to pass all these arguments around, param list could be
> > empty.
>
> No good reason. Your suggestion is better.
>
> > src/hotspot/share/classfile/vmSymbols.hpp
> >
> > Alignment of trailing '\' broken
>
> Oops!
>
> > src/java.base/share/classes/jdk/internal/misc/UnsafeConstants.java
> >
> > s/intitialize/initialize
> > s/iff/if
> 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" :)

..Thomas


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

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

[Andrew Dinn]

On 28/03/2019 12:17, Thomas Stüfe wrote:
> this looks fine, nits only:

Thank you for the review, Thomas. I'll post a follow up webrev to
address the comments. Responses are inline.

> UnsafeConstantsFixup::do_field()
> 
> you could shrink that coding a bit by factoring out the checks, e.g.:
> . . .

Yes, that was egregious cut-and-paste coding.

Actually, the idea is that all fields of the class are final static and
all of them are expected to be injected (which is why the final else
clause asserts).

So, I could just move the asserts up to precede the if-elseif cascade
and also include checks that the field is final and static.

  void do_field(fieldDescriptor* fd) {
oop mirror = fd->field_holder()->java_mirror();
assert(mirror != NULL, "UnsafeConstants must have mirror already");
assert(fd->field_holder() ==
SystemDictionary::UnsafeConstants_klass(), "Should be UnsafeConstants");
assert(fd->is_final(), "fields of UnsafeConstants must be final");
assert(fd->is_static() "fields of UnsafeConstants must be static");
if (fd->name() == vmSymbols::address_size_name()) {
  mirror->int_field_put(fd->offset(), _address_size);
} else if (fd->name() == vmSymbols::page_size_name()) {
  mirror->int_field_put(fd->offset(), _page_size);
} else if (fd->name() == vmSymbols::big_endian_name()) {
  . . .
} else {
  assert(false, "unexpected UnsafeConstants field");
}

> Also, is there a reason that code from thread.cpp:
> 
>     jint address_size = sizeof(void*);
>     jint page_size = os::vm_page_size();
>     jboolean is_big_endian = LITTLE_ENDIAN_ONLY(false)
> BIG_ENDIAN_ONLY(true);
>     jint use_unaligned_access = UseUnalignedAccesses;
> 
> could not just happen right in UnsafeConstantsFixup::do_field()? You
> would not have to pass all these arguments around, param list could be
> empty.

No good reason. Your suggestion is better.

> src/hotspot/share/classfile/vmSymbols.hpp
> 
> Alignment of trailing '\' broken

Oops!

> src/java.base/share/classes/jdk/internal/misc/UnsafeConstants.java
> 
> s/intitialize/initialize
> s/iff/if
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.

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

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

[Thomas Stüfe]

Hi,

this looks fine, nits only:

UnsafeConstantsFixup::do_field()

you could shrink that coding a bit by factoring out the checks, e.g.:

static oop mirror_with_checks_from_field_descriptor(fieldDescriptor* fd) {
 oop mirror = fd->field_holder()->java_mirror();
 assert(fd->field_holder() == SystemDictionary::UnsafeConstants_klass(),
"Should be UnsafeConstants");
 assert(mirror != NULL, "UnsafeConstants must have mirror already");
 return oop;
}

Also, is there a reason that code from thread.cpp:

jint address_size = sizeof(void*);
jint page_size = os::vm_page_size();
jboolean is_big_endian = LITTLE_ENDIAN_ONLY(false)
BIG_ENDIAN_ONLY(true);
jint use_unaligned_access = UseUnalignedAccesses;

could not just happen right in UnsafeConstantsFixup::do_field()? You would
not have to pass all these arguments around, param list could be empty.


src/hotspot/share/classfile/vmSymbols.hpp

Alignment of trailing '\' broken


src/java.base/share/classes/jdk/internal/misc/UnsafeConstants.java

s/intitialize/initialize
s/iff/if


Cheers, Thomas

On Thu, Mar 28, 2019 at 11:44 AM Andrew Dinn  wrote:

> Could I please have reviews for this patch which changes the
> initialization of four os/cpu-specific static final constants used by
> class Unsafe. The patch injects values during JVM startup (along similar
> lines to how String field COMPACT_STRINGS is initialized) rather than
> retrieving them via native method calls. This localizes the computation
> of the assigned values in one place, relocates the constants into a
> separate final, static-only Java class and avoids the need to maintain
> four separate native methods.
>
> A further motive for making this change is to pave the way for adding
> the writeback cache line size/address mask as Unsafe constants for use
> by the JEP proposed in JDK-8207851.
>
> JIRA:   https://bugs.openjdk.java.net/browse/JDK-8221477
> Webrev: http://cr.openjdk.java.net/~adinn/8221477/webrev.01
>
> Testing:
> submit repo tests passed
>
> 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
>
>
>