On Mon, 13 Mar 2023 18:51:17 GMT, Frederic Parain <fpar...@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
