On Wed, 8 Mar 2023 16:05:57 GMT, Coleen Phillimore <cole...@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.
>
> src/hotspot/share/classfile/fieldLayoutBuilder.cpp line 554:
> 
>> 552:     FieldInfo ctrl = _field_info->at(0);
>> 553:     FieldGroup* group = nullptr;
>> 554:     FieldInfo tfi = *it;
> 
> What's the 't' in tfi?  Maybe a longer variable name would be helpful here.

At some point there was a TempFieldInfo type, hence the name.
Renamed to fieldinfo.

> src/hotspot/share/classfile/javaClasses.cpp line 871:
> 
>> 869:       // a new UNSIGNED5 stream, and substitute it to the old FieldInfo 
>> stream.
>> 870: 
>> 871:       int java_fields;
> 
> Can you put InstanceKlass* ik = InstanceKlass::cast(k); here and use that so 
> there's only one cast?

Sure, done.

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

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

Reply via email to