On Wed, 15 Mar 2023 15:41: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: > > 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
