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

[Coleen Phillimore]

On Fri, 3 Mar 2023 14:50:34 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.

I was able to do a first pass through the vm code except for jvmci.  I didn't look at tests or SA in this pass.

src/hotspot/share/ci/ciFlags.hpp line 47:

> 45: 
> 46:   ciFlags()                  { _flags = 0; _stable = false; _intialized_final_update = false; }
> 47:   ciFlags(AccessFlags flags, bool is_stable= false, bool is_initialized_final_update = false) {

This should use constructor initializer syntax.

src/hotspot/share/classfile/classFileParser.cpp line 1491:

> 1489:   _temp_field_info = new GrowableArray<FieldInfo>(total_fields);
> 1490: 
> 1491:   ResourceMark rm(THREAD);

Is the ResourceMark ok here or should it go before allocating _temp_field_info ?

src/hotspot/share/classfile/classFileParser.cpp line 1608:

> 1606:       fflags.update_injected(true);
> 1607:       AccessFlags aflags;
> 1608:       FieldInfo fi(aflags, (u2)(injected[n].name_index), (u2)(injected[n].signature_index), 0, fflags);

I don't know why there's a cast here until I read more.  If the FieldInfo name_index and signature_index fields are only u2 sized, could you pass this as an int and then in the constructor assert that the value doesn't overflow u2 instead?

src/hotspot/share/classfile/classFileParser.cpp line 1634:

> 1632:       for(int i = 0; i < _temp_field_info->length(); i++) {
> 1633:         name = _temp_field_info->adr_at(i)->name(_cp);
> 1634:         sig = _temp_field_info->adr_at(i)->signature(_cp);

This checking for duplicates looks like a good candidate for a separate function because parse_fields is so long.  I'm adding this comment to remember to file an RFE to look into making this function shorter and factor out this code.

src/hotspot/share/classfile/classFileParser.cpp line 6024:

> 6022:   int injected_fields_count = _temp_field_info->length() - _java_fields_count;
> 6023:   _fieldinfo_stream = FieldInfoStream::create_FieldInfoStream(_temp_field_info, _java_fields_count, injected_fields_count, loader_data(), CHECK);
> 6024:   _fields_status = MetadataFactory::new_array<FieldStatus>(_loader_data, _temp_field_info->length(), FieldStatus(0), CHECK);

These lines seem long, could you reformat?

src/hotspot/share/classfile/fieldLayoutBuilder.cpp line 554:

> 552:     FieldInfo ctrl = _field_info->at(0);
> 553:     FieldGroup* group = nullptr;
> 554:     FieldInfo tfi = *it;

What's the 't' in tfi?  Maybe a longer variable name would be helpful here.

src/hotspot/share/classfile/javaClasses.cpp line 871:

> 869:       // a new UNSIGNED5 stream, and substitute it to the old FieldInfo stream.
> 870: 
> 871:       int java_fields;

Can you put InstanceKlass* ik = InstanceKlass::cast(k); here and use that so there's only one cast?

src/hotspot/share/classfile/javaClasses.cpp line 873:

> 871:       int java_fields;
> 872:       int injected_fields;
> 873:       GrowableArray<FieldInfo>* fields = FieldInfoStream::create_FieldInfoArray(InstanceKlass::cast(k)->fieldinfo_stream(), &java_fields, &injected_fields);

This line looks too long too.

src/hotspot/share/oops/fieldInfo.hpp line 31:

> 29: #include "memory/metadataFactory.hpp"
> 30: #include "oops/constantPool.hpp"
> 31: #include "oops/symbol.hpp"

Since you added an inline.hpp function can you move the functions that rely on including constantPool.hpp, symbol.hpp and metadataFactory.hpp into the inline.hpp file?

src/hotspot/share/oops/fieldInfo.hpp line 180:

> 178:   u2 generic_signature_index() const { return _generic_signature_index; }
> 179:   void set_generic_signature_index(u2 index) { _generic_signature_index = index; }
> 180:   u2 contention_group() const { return _contention_group; }

Can you align the { in these one line functions?

src/hotspot/share/oops/fieldStreams.hpp line 28:

> 26: #define SHARE_OOPS_FIELDSTREAMS_HPP
> 27: 
> 28: #include "oops/instanceKlass.inline.hpp"

including .inline.hpp from .hpp is against the guidelines.  You should move things and include instanceKlass.inline.hpp in fieldStreams.inline.hpp instead.

src/hotspot/share/oops/fieldStreams.hpp line 104:

> 102:     AccessFlags flags;
> 103:     flags.set_flags(field()->access_flags());
> 104:     return flags;

Did this used to do this for a reason?

src/hotspot/share/oops/fieldStreams.inline.hpp line 28:

> 26: #define SHARE_OOPS_FIELDSTREAMS_INLINE_HPP
> 27: 
> 28: #include "oops/fieldInfo.inline.hpp"

I don't know if you have to include oops/fieldInfo.inline.hpp but the include line for fieldStreams.hpp should be by itself and then this new include should be below with runtime/javaThread.hpp

src/hotspot/share/oops/instanceKlass.hpp line 275:

> 273:   // Fields information is stored in an UNSIGNED5 encoded stream (see fieldInfo.hpp)
> 274:   Array<u1>* _fieldinfo_stream;
> 275:   Array<FieldStatus>* _fields_status;

Can you align these two field identifiers?

src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 3582:

> 3580:   }
> 3581:   if (update_required) {
> 3582:     Array<u1>* old_stream = InstanceKlass::cast(scratch_class)->fieldinfo_stream();

scratch_class should already be an InstanceKlass, ie cast not required here or below.

src/hotspot/share/runtime/reflectionUtils.hpp line 29:

> 27: 
> 28: #include "memory/allStatic.hpp"
> 29: #include "oops/instanceKlass.inline.hpp"

Also here cannot include .inline.hpp in .hpp file.

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

Changes requested by coleenp (Reviewer).

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