On Fri, 3 Mar 2023 14:50:34 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. I was able to do a first pass through the vm code except for jvmci. I didn't look at tests or SA in this pass. src/hotspot/share/ci/ciFlags.hpp line 47: > 45: > 46: ciFlags() { _flags = 0; _stable = false; > _intialized_final_update = false; } > 47: ciFlags(AccessFlags flags, bool is_stable= false, bool > is_initialized_final_update = false) { This should use constructor initializer syntax. src/hotspot/share/classfile/classFileParser.cpp line 1491: > 1489: _temp_field_info = new GrowableArray<FieldInfo>(total_fields); > 1490: > 1491: ResourceMark rm(THREAD); Is the ResourceMark ok here or should it go before allocating _temp_field_info ? src/hotspot/share/classfile/classFileParser.cpp line 1608: > 1606: fflags.update_injected(true); > 1607: AccessFlags aflags; > 1608: FieldInfo fi(aflags, (u2)(injected[n].name_index), > (u2)(injected[n].signature_index), 0, fflags); I don't know why there's a cast here until I read more. If the FieldInfo name_index and signature_index fields are only u2 sized, could you pass this as an int and then in the constructor assert that the value doesn't overflow u2 instead? src/hotspot/share/classfile/classFileParser.cpp line 1634: > 1632: for(int i = 0; i < _temp_field_info->length(); i++) { > 1633: name = _temp_field_info->adr_at(i)->name(_cp); > 1634: sig = _temp_field_info->adr_at(i)->signature(_cp); This checking for duplicates looks like a good candidate for a separate function because parse_fields is so long. I'm adding this comment to remember to file an RFE to look into making this function shorter and factor out this code. src/hotspot/share/classfile/classFileParser.cpp line 6024: > 6022: int injected_fields_count = _temp_field_info->length() - > _java_fields_count; > 6023: _fieldinfo_stream = > FieldInfoStream::create_FieldInfoStream(_temp_field_info, _java_fields_count, > injected_fields_count, loader_data(), CHECK); > 6024: _fields_status = > MetadataFactory::new_array<FieldStatus>(_loader_data, > _temp_field_info->length(), FieldStatus(0), CHECK); These lines seem long, could you reformat? 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. 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? src/hotspot/share/classfile/javaClasses.cpp line 873: > 871: int java_fields; > 872: int injected_fields; > 873: GrowableArray<FieldInfo>* fields = > FieldInfoStream::create_FieldInfoArray(InstanceKlass::cast(k)->fieldinfo_stream(), > &java_fields, &injected_fields); This line looks too long too. src/hotspot/share/oops/fieldInfo.hpp line 31: > 29: #include "memory/metadataFactory.hpp" > 30: #include "oops/constantPool.hpp" > 31: #include "oops/symbol.hpp" Since you added an inline.hpp function can you move the functions that rely on including constantPool.hpp, symbol.hpp and metadataFactory.hpp into the inline.hpp file? src/hotspot/share/oops/fieldInfo.hpp line 180: > 178: u2 generic_signature_index() const { return _generic_signature_index; } > 179: void set_generic_signature_index(u2 index) { _generic_signature_index > = index; } > 180: u2 contention_group() const { return _contention_group; } Can you align the { in these one line functions? src/hotspot/share/oops/fieldStreams.hpp line 28: > 26: #define SHARE_OOPS_FIELDSTREAMS_HPP > 27: > 28: #include "oops/instanceKlass.inline.hpp" including .inline.hpp from .hpp is against the guidelines. You should move things and include instanceKlass.inline.hpp in fieldStreams.inline.hpp instead. src/hotspot/share/oops/fieldStreams.hpp line 104: > 102: AccessFlags flags; > 103: flags.set_flags(field()->access_flags()); > 104: return flags; Did this used to do this for a reason? src/hotspot/share/oops/fieldStreams.inline.hpp line 28: > 26: #define SHARE_OOPS_FIELDSTREAMS_INLINE_HPP > 27: > 28: #include "oops/fieldInfo.inline.hpp" I don't know if you have to include oops/fieldInfo.inline.hpp but the include line for fieldStreams.hpp should be by itself and then this new include should be below with runtime/javaThread.hpp src/hotspot/share/oops/instanceKlass.hpp line 275: > 273: // Fields information is stored in an UNSIGNED5 encoded stream (see > fieldInfo.hpp) > 274: Array<u1>* _fieldinfo_stream; > 275: Array<FieldStatus>* _fields_status; Can you align these two field identifiers? src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 3582: > 3580: } > 3581: if (update_required) { > 3582: Array<u1>* old_stream = > InstanceKlass::cast(scratch_class)->fieldinfo_stream(); scratch_class should already be an InstanceKlass, ie cast not required here or below. src/hotspot/share/runtime/reflectionUtils.hpp line 29: > 27: > 28: #include "memory/allStatic.hpp" > 29: #include "oops/instanceKlass.inline.hpp" Also here cannot include .inline.hpp in .hpp file. ------------- Changes requested by coleenp (Reviewer). PR: https://git.openjdk.org/jdk/pull/12855
