RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v5]

[David Holmes]

On Wed, 15 Mar 2023 15:41:17 GMT, Frederic Parain <fparain at openjdk.org> wrote:

>> Please review this change re-implementing the FieldInfo data structure.
>> 
>> The FieldInfo array is an old data structure storing fields metadata. It has poor extension capabilities, a complex management code because of lack of strong typing and semantic overloading, and a poor memory efficiency.
>> 
>> The new implementation uses a compressed stream to store those metadata, achieving better memory density and providing flexible extensibility, while exposing a strongly typed set of data when uncompressed. The stream is compressed using the unsigned5 encoding, which alreay present in the JDK (because of pack200) and the JVM (because JIT compulers use it to comrpess debugging information).
>> 
>> More technical details are available in the CR: https://bugs.openjdk.org/browse/JDK-8292818
>> 
>> Those changes include a re-organisation of fields' flags, splitting the previous heterogeneous AccessFlags field into three distincts flag categories: immutable flags from the class file, immutable fields defined by the JVM, and finally mutable flags defined by the JVM.
>> 
>> The SA, CI, and JVMCI, which all used to access the old FieldInfo array, have been updated too to deal with the new FieldInfo format.
>> 
>> Tested with mach5, tier 1 to 7.
>> 
>> Thank you.
>
> Frederic Parain has updated the pull request incrementally with one additional commit since the last revision:
> 
>   SA and JVMCI fixes

Nice piece of work Fred - I won't pretend to follow every detail.

A few nits on unnecessary alignment (which may match pre-existing style not evident in the diff).

Thanks.

src/hotspot/share/oops/fieldInfo.inline.hpp line 170:

> 168:     new_flags = old_flags & ~mask;
> 169:     witness = Atomic::cmpxchg(&flags, old_flags, new_flags);
> 170:   } while(witness != old_flags);

Just to prove I did read this :) space needed after `while`

src/hotspot/share/oops/fieldInfo.inline.hpp line 174:

> 172: 
> 173: inline void FieldStatus::update_flag(FieldStatusBitPosition pos, bool z) {
> 174:   if (z)    atomic_set_bits(  _flags, flag_mask(pos));

Nit: extra space before `_flags`

src/hotspot/share/oops/fieldInfo.inline.hpp line 175:

> 173: inline void FieldStatus::update_flag(FieldStatusBitPosition pos, bool z) {
> 174:   if (z)    atomic_set_bits(  _flags, flag_mask(pos));
> 175:   else      atomic_clear_bits(_flags, flag_mask(pos));

Nit: no need for the extra spaces. If you really want these to align just place them on ne wlines.

src/hotspot/share/oops/instanceKlass.inline.hpp line 50:

> 48: 
> 49: inline Symbol* InstanceKlass::field_name        (int index) const { return field(index).name(constants()); }
> 50: inline Symbol* InstanceKlass::field_signature   (int index) const { return field(index).signature(constants()); }

There should not be spaces between a method name and the opening `(`. I'm really not a fine of this kind of alignment.

-------------

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.org/jdk/pull/12855