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

Thu Apr 4 00:25:34 UTC 2019

Hi,  Sorry for the delayed reply.

On 4/2/19 10:51 AM, Andrew Dinn wrote:
> 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.

Yes, I see the advantage to it.  I wish it were more compact but it's 
fine the way it you've written it.
>
>> + 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.

Please, fix that one too.  I've had several passes removing 
InstanceKlass::cast()s but they keep sneaking in.
>
>> 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.

Actually the field names are fine, I don't like that we have to add them 
to vmSymbols.hpp.  But that's how the code works.

Thank you for the rationale of the naming though.  It makes sense.

Coleen

> 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