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

[Doug Simon]

On Mon, 13 Mar 2023 18:51: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:
> 
>   Fixes includes and style

src/hotspot/share/jvmci/jvmciEnv.cpp line 1439:

> 1437:     JNIAccessMark jni(this, THREAD);
> 1438:     jobject result = jni()->NewObject(JNIJVMCI::FieldInfo::clazz(),
> 1439:                                       JNIJVMCI::VMFlag::constructor(),

`JNIJVMCI::VMFlag::constructor()` is the wrong constructor.

src/hotspot/share/jvmci/jvmciEnv.hpp line 149:

> 147: };
> 148: 
> 149: extern JNIEXPORT jobjectArray c2v_getDeclaredFieldsInfo(JNIEnv* env, jobject, jobject, jlong);

What's the purpose of this declaration? I don't think you need it or the `friend` declaration below since `new_FieldInfo(FieldInfo* fieldinfo, JVMCI_TRAPS)` is public.

src/hotspot/share/jvmci/vmStructs_jvmci.cpp line 416:

> 414:   declare_constant(FieldInfo::FieldFlags::_ff_injected)                   \
> 415:   declare_constant(FieldInfo::FieldFlags::_ff_stable)                     \
> 416:   declare_constant(FieldInfo::FieldFlags::_ff_generic)                    \

I don't think `_ff_generic` is used in the JVMCI Java code so this entry can be deleted. Please double check the other entries.

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotConstantPool.java line 814:

> 812:             HotSpotResolvedObjectTypeImpl resolvedHolder;
> 813:             try {
> 814:                 resolvedHolder = compilerToVM().resolveFieldInPool(this, index, (HotSpotResolvedJavaMethodImpl) method, (byte) opcode, info);

Please update the javadoc for `CompilerToVM.resolveFieldInPool` to reflect the expanded definition of `info`.

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotResolvedObjectTypeImpl.java line 88:

> 86: 
> 87:     /**
> 88:      * Lazily initialized cache for FieldInfo

nit: missing `.` at end of sentence

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/meta/ResolvedJavaField.java line 48:

> 46:      * Returns VM internal flags associated with this field
> 47:      */
> 48:     int getInternalModifiers();

We've never exposed the internal modifiers before in public JVMCI API and we should refrain from doing so until there's a good reason to do so. Please remove this method.

test/hotspot/jtreg/compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestResolvedJavaField.java line 97:

> 95: 
> 96:     @Test
> 97:     public void getInternalModifiersTest() {

No need for this test since the `getInternalModifiers` method should be removed.

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

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