Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v5]
On Wed, 15 Mar 2023 15:41: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:
>
> 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(, 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
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v5]
On Wed, 15 Mar 2023 15:41: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: > > SA and JVMCI fixes Marked as reviewed by dnsimon (Committer). - PR: https://git.openjdk.org/jdk/pull/12855
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v5]
On Wed, 15 Mar 2023 15:41: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: > > SA and JVMCI fixes SA changes looks good. Thanks for taking care of this! - Marked as reviewed by cjplummer (Reviewer). PR: https://git.openjdk.org/jdk/pull/12855
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v5]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/12855/files - new: https://git.openjdk.org/jdk/pull/12855/files/12b4f1b4..f81337f7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12855=04 - incr: https://webrevs.openjdk.org/?repo=jdk=12855=03-04 Stats: 130 lines in 13 files changed: 14 ins; 46 del; 70 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
