Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v4]
On Tue, 14 Mar 2023 01:25:01 GMT, Chris Plummer wrote:
>> Frederic Parain has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fixes includes and style
>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/Field.java line 75:
>
>> 73: int initialValueIndex;
>> 74: int genericSignatureIndex;
>> 75: int contendedGroup;
>
> It seems that these should all be shorts. All the getter methods are casting
> them to short.
Indexes in the constant pool are unsigned shorts, but Java shorts are signed,
using ints is the simplest way to store those indexes.
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/InstanceKlass.java
> line 108:
>
>> 106: CLASS_STATE_INITIALIZATION_ERROR =
>> db.lookupIntConstant("InstanceKlass::initialization_error").intValue();
>> 107: // We need a new fieldsCache each time we attach.
>> 108: fieldsCache = new HashMap();
>
> This should probably be a WeakHashMap. I tried it and it seems to work (or at
> least didn't cause any problems). However, when doing a heap dump I didn't
> notice the table being any smaller on exit when it was made weak, even though
> there were numerous GC's while dumping the heap.
>
> The `` is the Address of the hotspot InstanceKlass instance, and this
> Address is referenced by the SA InstanceKlass mirror. So theoretically when
> the reference to the mirror goes way, then the cache entry can be cleared.
I've changed the map to a WeakHashMap and didn't see any issue during testing.
But I didn't measure footprint.
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/InstanceKlass.java
> line 325:
>
>> 323:
>> 324: public int getFieldOffset(int index) {
>> 325: return (int)getField(index).getOffset();
>
> Cast to int is not needed
Other APIs (like MetadaField) are using longs to pass offsets, doing a cast
here is less disruptive than changing all the other APIs.
-
PR: https://git.openjdk.org/jdk/pull/12855
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v4]
On Tue, 14 Mar 2023 15:10:04 GMT, Doug Simon wrote: >> Access to internal modifiers is needed in >> `HotSpotResolvedJavaFieldTest.testEquivalenceForInternalFields()`. I moved >> the declaration of the method to `HotSpotResolvedJavaField`. Does this >> change work for you? > > Just use reflection to read the internal flags (like this test already does > for the `index` field). > > I've attached > [review.patch](https://github.com/openjdk/jdk/files/10970245/review.patch) > with this change and a few other changes I think should be made for better > naming (plus one test cleanup). Thank you for the patch, it will be included in the next commit. - PR: https://git.openjdk.org/jdk/pull/12855
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v4]
On Tue, 14 Mar 2023 15:11:36 GMT, Doug Simon wrote: >> Without this declaration, builds fail on Windows with this error: >> `error C2375: 'c2v_getDeclaredFieldsInfo': redefinition; different linkage` > > Strange - thats not needed for other `JVMCIEnv` methods called from > `jvmciCompilerToVM.cpp`. There must be some way to avoid this. The issue was caused by the `friend` declaration below (I cannot remember why I added in the first place), which seems to add an implicit declaration of the method that was conflicting with the original declaration of the method. Once the `friend` declaration is removed, builds on Windows don't need the `extern` declaration anymore. - PR: https://git.openjdk.org/jdk/pull/12855
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v4]
On Tue, 14 Mar 2023 13:37:23 GMT, Frederic Parain wrote: >> 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. > > Without this declaration, builds fail on Windows with this error: > `error C2375: 'c2v_getDeclaredFieldsInfo': redefinition; different linkage` Strange - thats not needed for other `JVMCIEnv` methods called from `jvmciCompilerToVM.cpp`. There must be some way to avoid this. - PR: https://git.openjdk.org/jdk/pull/12855
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v4]
On Tue, 14 Mar 2023 13:12:31 GMT, Frederic Parain wrote: >> 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. > > Access to internal modifiers is needed in > `HotSpotResolvedJavaFieldTest.testEquivalenceForInternalFields()`. I moved > the declaration of the method to `HotSpotResolvedJavaField`. Does this change > work for you? Just use reflection to read the internal flags (like this test already does for the `index` field). I've attached [review.patch](https://github.com/openjdk/jdk/files/10970245/review.patch) with this change and a few other changes I think should be made for better naming (plus one test cleanup). - PR: https://git.openjdk.org/jdk/pull/12855
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v4]
On Mon, 13 Mar 2023 21:35:17 GMT, Doug Simon wrote: >> 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.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. Without this declaration, builds fail on Windows with this error: `error C2375: 'c2v_getDeclaredFieldsInfo': redefinition; different linkage` - PR: https://git.openjdk.org/jdk/pull/12855
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v4]
On Mon, 13 Mar 2023 21:44:59 GMT, Doug Simon wrote: >> Frederic Parain has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fixes includes and style > > 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. _ff_generic removed. _ff_stable is used in `HotSpotResolvedJavaFieldImpl.isStable()`. _ff_injected is used in `HotSpotResolvedJavaFieldImpl.isInternal()` - PR: https://git.openjdk.org/jdk/pull/12855
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v4]
On Mon, 13 Mar 2023 21:53:37 GMT, Doug Simon wrote: >> Frederic Parain has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fixes includes and style > > 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. Access to internal modifiers is needed in `HotSpotResolvedJavaFieldTest.testEquivalenceForInternalFields()`. I moved the declaration of the method to `HotSpotResolvedJavaField`. Does this change work for you? - PR: https://git.openjdk.org/jdk/pull/12855
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v4]
On Mon, 13 Mar 2023 18:51:17 GMT, Frederic Parain 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/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/InstanceKlass.java
line 108:
> 106: CLASS_STATE_INITIALIZATION_ERROR =
> db.lookupIntConstant("InstanceKlass::initialization_error").intValue();
> 107: // We need a new fieldsCache each time we attach.
> 108: fieldsCache = new HashMap();
This should probably be a WeakHashMap. I tried it and it seems to work (or at
least didn't cause any problems). However, when doing a heap dump I didn't
notice the table being any smaller on exit when it was made weak, even though
there were numerous GC's while dumping the heap.
The is the Address of the hotspot InstanceKlass instance, and this
Address is referenced by the SA InstanceKlass mirror. So theoretically when the
reference to the mirror goes way, then the cache entry can be cleared.
-
PR: https://git.openjdk.org/jdk/pull/12855
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v4]
On Mon, 13 Mar 2023 18:51:17 GMT, Frederic Parain 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
Changes requested by cjplummer (Reviewer).
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/Field.java line 75:
> 73: int initialValueIndex;
> 74: int genericSignatureIndex;
> 75: int contendedGroup;
It seems that these should all be shorts. All the getter methods are casting
them to short.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/Field.java line 99:
> 97: if (fieldIsInitialized(fieldInfoValues.fieldFlags))
> fieldInfoValues.initialValueIndex = crs.readInt(); // read initial value index
> 98: if (fieldIsGeneric(fieldInfoValues.fieldFlags))
> fieldInfoValues.genericSignatureIndex = crs.readInt(); // read generic
> signature index
> 99: if (fieldIsContended(fieldInfoValues.fieldFlags))
> fieldInfoValues.contendedGroup = crs.readInt(); // read contended group
Column with is too wide. These lines would be easier to read if you made each
one multiple lines with curly braces.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/Field.java line 107:
> 105: int javafieldsCount = crs.readInt(); // read num_java_fields
> 106: int VMFieldsCount = crs.readInt(); // read num_injected_fields;
> 107: int numFields = javafieldsCount + VMFieldsCount;
VMFieldsCount -> vmFieldsCount, or maybe just use num_java_fields and
num_injected_fields
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/InstanceKlass.java
line 285:
> 283: public short getFieldNameIndex(int index) {
> 284: if (index >= getJavaFieldsCount()) throw new
> IndexOutOfBoundsException("not a Java field;");
> 285: return (short)getField(index).getNameIndex();
Cast to short not needed
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/InstanceKlass.java
line 303:
> 301: public short getFieldSignatureIndex(int index) {
> 302: if (index >= getJavaFieldsCount()) throw new
> IndexOutOfBoundsException("not a Java field;");
> 303: return (short)getField(index).getGenericSignatureIndex();
Cast to short is not needed
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/InstanceKlass.java
line 321:
> 319: public short getFieldInitialValueIndex(int index) {
> 320: if (index >= getJavaFieldsCount()) throw new
> IndexOutOfBoundsException("not a Java field;");
> 321: return (short)getField(index).getInitialValueIndex();
cast to short is not needed
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/InstanceKlass.java
line 325:
> 323:
> 324: public int getFieldOffset(int index) {
> 325: return (int)getField(index).getOffset();
Cast to int is not needed
-
PR: https://git.openjdk.org/jdk/pull/12855
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v4]
On Mon, 13 Mar 2023 18:51:17 GMT, Frederic Parain 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
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v4]
On Mon, 13 Mar 2023 18:51:17 GMT, Frederic Parain 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 All my comments are addressed. Thank you! This is significant work. - Marked as reviewed by coleenp (Reviewer). PR: https://git.openjdk.org/jdk/pull/12855
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v4]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/12855/files - new: https://git.openjdk.org/jdk/pull/12855/files/322b626d..12b4f1b4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12855=03 - incr: https://webrevs.openjdk.org/?repo=jdk=12855=02-03 Stats: 9 lines in 5 files changed: 3 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/12855.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12855/head:pull/12855 PR: https://git.openjdk.org/jdk/pull/12855
