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

[Andrew Dinn]

Hi Colleen,

thanks for looking at this.

On 01/04/2019 14:28, coleen.phillimore at oracle.com wrote:
> 
> http://cr.openjdk.java.net/~adinn/8221477/webrev.02/src/hotspot/share/classfile/javaClasses.cpp.udiff.html
> 
> 
> I was sort of expecting you to use like UNSAFE_CONSTANTS_DO for the
> fields, so that you don't have to add more names to vmSymbols.hpp. And
> that's what the rest of the code does (except CompactStrings). I can
> sort of  understand that it's faster as static_fields_do, and you can
> check for unexpected fields.

Well, I'm only following the model used for CompactStrings because what
Vladimir (Kozlov) suggested it as a model to follow when reviewing JEP
352 (it was suggested as a way to initialize the fields I will need to
size/align cache lines). I'm not especially attached to this way of
doing it.

That said, I understand what you say about this vs the alternative.
Implementing an UNSAFE_CONSTANTS_DO macro would, on the plus side, avoid
the need to add those extra symbols into vmSymbols.hpp but, on the flip
side, would

  i) require repeated calls to InstanceKlass::find_local_field
  ii) check for expected fields but not detect unexpected fields

Is there a reason to be especially concerned over extra field name
symbols? I'll change to UNSAFE_CONSTANTS_DO if you want but I really
like the fact that this way of doing it guarantees that /all/ fields are
final, static and expected.

> + jint _address_size;
> + jint _page_size;
> + jboolean _big_endian;
> + jboolean _use_unaligned_access;
> 
> We've been trying to keep Java types out of the C++ code, and use the
> appropriate C++ types.

Sure, I'll update this accordingly.

> InstanceKlass::cast(SystemDictionary::UnsafeConstants_klass())->do_local_static_fields(&fixup);

> The cast is unnecessary.  UnsafeConstants_klass returns an InstanceKlass.

Ah, ok, I cut and pasted this from the equivalent code for String.
Perhaps I should fix that as well.

> http://cr.openjdk.java.net/~adinn/8221477/webrev.02/src/hotspot/share/classfile/vmSymbols.hpp.udiff.html
> 
> 
> I just don't like the field names here. :(
Well, sorry, but I have to ask do you mean the words or the format? If
the words then is that the names on the left (address_size_name etc) or
names on the right (ADDRESS_SIZE etc). n.b as regards upper case I chose
ALL_CAPS because these are final static constant numeric fields and ...

 ... well, I am pleased you brought this up. There is a 'semi-lucid'
rationale for the current names albeit one which I am not 100% happy with:

I tried to retain the naming convention which is (sort-of) followed in
Unsafe e.g. field ADDRESS_SIZE caches the value retrieved by native
method addressSize0 and is exposed by public getter addressSize(). I
hoped to define all the relevant constant fields in UnsafeConstants
using names originally found in class Unsafe. The idea was then to
delete the Unsafe fields and statically import the ones from
UnsafeConstants into class Unsafe. That way I a simple mention of the
imported constants in the getter or at other internal points where they
were used would continue to work.

That's fine for PAGE_SIZE, UNALIGNED_ACCESS and BE. Getter method
pageSize() was always implemented as native with no cached value. So,
the old native method can simply be redefined to return the value
injected into UnsafeConstants. PAGE_SIZE is thus the conventional name
for the static field.

The value returned by unalignedAccess0 was cached in private field
unalignedAccess (ah yes, camelCase :-/) and then exposed by public
getter unalignedAccess(). So, once again importing a field in
UnsafeConstants called UNALIGNED_ACCESS and returning this form the
getter is uniform.

Method isBigEndian0 was used to initialize a field called BE. So, I
could have followed suit and called the field in UnsafeConstants BE and
this would have worked. However, I though a longer, more explicit name
for the UnsafeConstants field would be clearer and picked BIG_ENDIAN.
However, I decided to leave the original field BE in Unsafe and simply
initialize it from BIG_ENDIAN because it is used in many ternary
expression (BE ? ... : ...)  and changing it to use a longer name looked
terribly messy. So, I admit the choice of BIG_ENDIAN is a little
perverse and strictly this field ought to be called BE.

However, that turned out to be somewhat moot as the plan was foiled by a
further difficulty. I could not delete field Unsafe::ADDRESS_SIZE and
import UnsafeConstants::ADDRESS_SIZE because the the field in Unsafe is
public (what is worse, it is referenced from the JDK runtime even though
public getter addressSize exists). So, the original idea of deleting the
Unsafe fields and importing the UnsafeConstants equivalents under the
same names was snookered from the get go. In consequence, I simply
import class UnsafeConstants and reference the constants with an
explicit classname qualifier.

So, I accept this is far from perfect. If you have a preferred
suggestion as to how to name the fields and getters in UnsafeConstants
and/or Unsafe or just advice on how to revise/improve the choices I have
made I'd be happy to hear it.

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