-
Notifications
You must be signed in to change notification settings - Fork 127
8370450: [lworld] Alternate implementation of the substitutability test method #1695
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
23c2ccd
893afdc
c4d5cf4
22b7404
661ffa7
cf456a9
f5f7448
e9e79fe
52959d0
99f2114
899f480
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1383,6 +1383,7 @@ void ClassFileParser::parse_fields(const ClassFileStream* const cfs, | |
| assert(nullptr == _fields_type_annotations, "invariant"); | ||
|
|
||
| bool is_inline_type = !class_access_flags.is_identity_class() && !class_access_flags.is_abstract(); | ||
| bool is_value_class = !class_access_flags.is_identity_class() && !class_access_flags.is_interface(); | ||
| cfs->guarantee_more(2, CHECK); // length | ||
| const u2 length = cfs->get_u2_fast(); | ||
| *java_fields_count_ptr = length; | ||
|
|
@@ -1394,7 +1395,7 @@ void ClassFileParser::parse_fields(const ClassFileStream* const cfs, | |
| // two more slots are required for inline classes: | ||
| // one for the static field with a reference to the pre-allocated default value | ||
| // one for the field the JVM injects when detecting an empty inline class | ||
| const int total_fields = length + num_injected + (is_inline_type ? 2 : 0); | ||
| const int total_fields = length + num_injected + (is_inline_type ? 2 : 0) + (is_value_class ? 1 : 0); | ||
|
|
||
| // Allocate a temporary resource array to collect field data. | ||
| // After parsing all fields, data are stored in a UNSIGNED5 compressed stream. | ||
|
|
@@ -1575,6 +1576,22 @@ void ClassFileParser::parse_fields(const ClassFileStream* const cfs, | |
| _temp_field_info->adr_at(idx2)->set_index(idx2); | ||
| _static_oop_count++; | ||
| } | ||
| if (!access_flags().is_identity_class() && !access_flags().is_interface() | ||
| && _class_name != vmSymbols::java_lang_Object()) { | ||
| // Acmp map required for abstract and concrete value classes | ||
| FieldInfo::FieldFlags fflags2(0); | ||
| fflags2.update_injected(true); | ||
| fflags2.update_stable(true); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If c2 can model the field map read as a stable array read, this allows ValueObjectMethods to generate efficient code for specialized value types in the future. 👍 |
||
| AccessFlags aflags2(JVM_ACC_STATIC); | ||
| FieldInfo fi3(aflags2, | ||
| (u2)vmSymbols::as_int(VM_SYMBOL_ENUM_NAME(acmp_maps_name)), | ||
| (u2)vmSymbols::as_int(VM_SYMBOL_ENUM_NAME(int_array_signature)), | ||
| 0, | ||
| fflags2); | ||
| int idx2 = _temp_field_info->append(fi3); | ||
| _temp_field_info->adr_at(idx2)->set_index(idx2); | ||
| _static_oop_count++; | ||
| } | ||
|
|
||
| if (_need_verify && length > 1) { | ||
| // Check duplicated fields | ||
|
|
@@ -5520,6 +5537,40 @@ void ClassFileParser::fill_instance_klass(InstanceKlass* ik, | |
| vk->initialize_calling_convention(CHECK); | ||
| } | ||
|
|
||
| if (EnableValhalla && !access_flags().is_identity_class() && !access_flags().is_interface() | ||
| && _class_name != vmSymbols::java_lang_Object()) { | ||
| // Both abstract and concrete value classes need a field map for acmp | ||
| ik->set_acmp_maps_offset(_layout_info->_acmp_maps_offset); | ||
| // Current format of acmp maps: | ||
| // All maps are stored contiguously in a single int array because it might | ||
| // be to early to instantiate an Object array (to be investigated) | ||
fparain marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| // Format is: | ||
| // [number_of_nonoop_entries][offset0][size[0][offset1][size1]...[oop_offset0][oop_offset1]... | ||
| // ^ ^ | ||
| // | | | ||
| // --------------------- Pair of integer describing a segment of | ||
| // contiguous non-oop fields | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the ascii art - very helpful. |
||
| // First element is the number of segment of contiguous non-oop fields | ||
| // Then, each segment of contiguous non-oop fields is described by two consecutive elements: | ||
| // the offset then the size. | ||
| // After the last segment of contiguous non-oop fields, oop fields are described, one element | ||
| // per oop field, containing the offset of the field. | ||
| int nonoop_acmp_map_size = _layout_info->_nonoop_acmp_map->length() * 2; | ||
| int oop_acmp_map_size = _layout_info->_oop_acmp_map->length(); | ||
| typeArrayOop map = oopFactory::new_intArray(nonoop_acmp_map_size + oop_acmp_map_size + 1, CHECK); | ||
| typeArrayHandle map_h(THREAD, map); | ||
| map_h->int_at_put(0, _layout_info->_nonoop_acmp_map->length()); | ||
| for (int i = 0; i < _layout_info->_nonoop_acmp_map->length(); i++) { | ||
| map_h->int_at_put(i * 2 + 1, _layout_info->_nonoop_acmp_map->at(i).first); | ||
| map_h->int_at_put(i * 2 + 2, _layout_info->_nonoop_acmp_map->at(i).second); | ||
| } | ||
| int oop_map_start = nonoop_acmp_map_size + 1; | ||
| for (int i = 0; i < _layout_info->_oop_acmp_map->length(); i++) { | ||
| map_h->int_at_put(oop_map_start + i, _layout_info->_oop_acmp_map->at(i)); | ||
| } | ||
| ik->java_mirror()->obj_field_put(ik->acmp_maps_offset(), map_h()); | ||
| } | ||
|
|
||
| ClassLoadingService::notify_class_loaded(ik, false /* not shared class */); | ||
|
|
||
| if (!is_internal()) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -211,6 +211,7 @@ FieldLayout::FieldLayout(GrowableArray<FieldInfo>* field_info, Array<InlineLayou | |
| _super_alignment(-1), | ||
| _super_min_align_required(-1), | ||
| _null_reset_value_offset(-1), | ||
| _acmp_maps_offset(-1), | ||
| _super_has_fields(false), | ||
| _has_inherited_fields(false) {} | ||
|
|
||
|
|
@@ -403,6 +404,9 @@ LayoutRawBlock* FieldLayout::insert_field_block(LayoutRawBlock* slot, LayoutRawB | |
| if (_field_info->adr_at(block->field_index())->name(_cp) == vmSymbols::null_reset_value_name()) { | ||
| _null_reset_value_offset = block->offset(); | ||
| } | ||
| if (_field_info->adr_at(block->field_index())->name(_cp) == vmSymbols::acmp_maps_name()) { | ||
| _acmp_maps_offset = block->offset(); | ||
| } | ||
| } | ||
| if (block->block_kind() == LayoutRawBlock::FLAT && block->layout_kind() == LayoutKind::NULLABLE_ATOMIC_FLAT) { | ||
| int nm_offset = block->inline_klass()->null_marker_offset() - block->inline_klass()->payload_offset() + block->offset(); | ||
|
|
@@ -1249,6 +1253,7 @@ void FieldLayoutBuilder::compute_inline_class_layout() { | |
| _static_layout->add(_static_fields->big_primitive_fields()); | ||
| _static_layout->add(_static_fields->small_primitive_fields()); | ||
|
|
||
| generate_acmp_maps(); | ||
| epilogue(); | ||
| } | ||
|
|
||
|
|
@@ -1288,6 +1293,152 @@ void FieldLayoutBuilder::register_embedded_oops(OopMapBlocksBuilder* nonstatic_o | |
| register_embedded_oops_from_list(nonstatic_oop_maps, group->small_primitive_fields()); | ||
| } | ||
|
|
||
| static int insert_segment(GrowableArray<Pair<int,int>>* map, int offset, int size, int last_idx) { | ||
| if (map->is_empty()) { | ||
| return map->append(Pair<int,int>(offset, size)); | ||
| } | ||
| last_idx = last_idx == -1 ? 0 : last_idx; | ||
| int start = map->adr_at(last_idx)->first > offset ? 0 : last_idx; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. last_idx is an optimization to try to prevent searching through the field map to find where to insert {offset,size} for the new entry. When would it not be ascending? super classes and inlined classes are inserted in order that they're found in the LayoutBlocks so wouldn't the offsets be ascending?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You answered this in slack for me. Can you put this at the top of this function:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comments have been added in |
||
| bool inserted = false; | ||
| for (int c = start; c < map->length() && !inserted ; c++) { | ||
fparain marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| if (offset == (map->adr_at(c)->first + map->adr_at(c)->second)) { | ||
| //contiguous to the last field, can be coalesced | ||
| map->adr_at(c)->second = map->adr_at(c)->second + size; | ||
| inserted = true; | ||
| break; // break out of the for loop | ||
| } | ||
| if (offset < (map->adr_at(c)->first)) { | ||
| map->insert_before(c, Pair<int,int>(offset, size)); | ||
| last_idx = c; | ||
| inserted = true; | ||
| break; // break out of the for loop | ||
| } | ||
| } | ||
| if (!inserted) { | ||
| last_idx = map->append(Pair<int,int>(offset, size)); | ||
| } | ||
| return last_idx; | ||
| } | ||
|
|
||
| static int insert_map_at_offset(GrowableArray<Pair<int,int>>* nonoop_map, GrowableArray<int>* oop_map, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This copies the contents of the maps from an inline class that's flattened into this class or a super class into the map for this class. |
||
| const InstanceKlass* ik, int offset, int payload_offset, int last_idx) { | ||
| oop mirror = ik->java_mirror(); | ||
| oop array = mirror->obj_field(ik->acmp_maps_offset()); | ||
| assert(array != nullptr, "Sanity check"); | ||
| typeArrayOop fmap = (typeArrayOop)array; | ||
| typeArrayHandle fmap_h(Thread::current(), fmap); | ||
| int nb_nonoop_field = fmap_h->int_at(0); | ||
| int field_offset = offset - payload_offset; | ||
| for (int i = 0; i < nb_nonoop_field; i++) { | ||
| last_idx = insert_segment(nonoop_map, | ||
| field_offset + fmap_h->int_at( i * 2 + 1), | ||
| fmap_h->int_at( i * 2 + 2), last_idx); | ||
| } | ||
| int len = fmap_h->length(); | ||
| for (int i = nb_nonoop_field * 2 + 1; i < len; i++) { | ||
| oop_map->append(field_offset + fmap_h->int_at(i)); | ||
| } | ||
| return last_idx; | ||
| } | ||
|
|
||
| static void split_after(GrowableArray<Pair<int,int>>* map, int idx, int head) { | ||
| int offset = map->adr_at(idx)->first; | ||
| int size = map->adr_at(idx)->second; | ||
| if (size <= head) return; | ||
| map->adr_at(idx)->first = offset + head; | ||
| map->adr_at(idx)->second = size - head; | ||
| map->insert_before(idx, Pair<int,int>(offset, head)); | ||
|
|
||
| } | ||
|
|
||
| void FieldLayoutBuilder::generate_acmp_maps() { | ||
| assert(_is_inline_type || _is_abstract_value, "Must be done only for value classes (abstract or not)"); | ||
|
|
||
| // create/initialize current class' maps | ||
| // The Pair<int,int> values in the nonoop_acmp_map represent <offset,size> segments of memory | ||
| _nonoop_acmp_map = new GrowableArray<Pair<int,int>>(); | ||
| _oop_acmp_map = new GrowableArray<int>(); | ||
| if (_is_empty_inline_class) return; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not return first before allocating these GrowableArray in resource area?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It streamlines the code in InstanceKlass::fill_instance_klass() by not having to handle the null map case.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay. I can't decide if you need a comment to prevent someone from changing this to have the quick exit. I guess they'll get a crash for the null case. |
||
| int last_idx = -1; | ||
| if (_super_klass != nullptr && _super_klass != vmClasses::Object_klass()) { // Assumes j.l.Object cannot have fields | ||
| last_idx = insert_map_at_offset(_nonoop_acmp_map, _oop_acmp_map, _super_klass, 0, 0, last_idx); | ||
| } | ||
|
|
||
| // Processing local fields | ||
| LayoutRawBlock* b = _layout->blocks(); | ||
| while(b != _layout->last_block()) { | ||
| switch(b->block_kind()) { | ||
| case LayoutRawBlock::RESERVED: | ||
| case LayoutRawBlock::EMPTY: | ||
| case LayoutRawBlock::PADDING: | ||
| case LayoutRawBlock::NULL_MARKER: | ||
| case LayoutRawBlock::INHERITED: // inherited fields are handled during maps creation/initialization | ||
| // skip | ||
| break; | ||
|
|
||
| case LayoutRawBlock::REGULAR: | ||
| { | ||
| FieldInfo* fi = _field_info->adr_at(b->field_index()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there an assumption that the block field indexes are in ascending order?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I understand the question.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that was the question. I meant to ask if the offsets are in ascending order, so that makes the code to coalesce them simpler. Can you add a comment at 1367 that the field offsets are in ascending order and that makes reading this easier. |
||
| if (fi->signature(_constant_pool)->starts_with("L") || fi->signature(_constant_pool)->starts_with("[")) { | ||
| _oop_acmp_map->append(b->offset()); | ||
| } else { | ||
| // Non-oop case | ||
| last_idx = insert_segment(_nonoop_acmp_map, b->offset(), b->size(), last_idx); | ||
| } | ||
| break; | ||
| } | ||
| case LayoutRawBlock::FLAT: | ||
| { | ||
| InlineKlass* vk = b->inline_klass(); | ||
| last_idx = insert_map_at_offset(_nonoop_acmp_map, _oop_acmp_map, vk, b->offset(), vk->payload_offset(), last_idx); | ||
| if (b->layout_kind() == LayoutKind::NULLABLE_ATOMIC_FLAT) { | ||
| int null_marker_offset = b->offset() + vk->null_marker_offset_in_payload(); | ||
| last_idx = insert_segment(_nonoop_acmp_map, null_marker_offset, 1, last_idx); | ||
| // Important note: the implementation assumes that for nullable flat fields, if the | ||
| // null marker is zero (field is null), then all the fields of the flat field are also | ||
| // zeroed. So, nullable flat field are not encoded different than null-free flat fields, | ||
| // all fields are included in the map, plus the null marker | ||
| // If it happens that the assumption above is wrong, then nullable flat fields would | ||
| // require a dedicated section in the acmp map, and be handled differently: null_marker | ||
| // comparison first, and if null markers are identical and non-zero, then conditional | ||
| // comparison of the other fields | ||
| } | ||
| } | ||
| break; | ||
|
|
||
| } | ||
| b = b->next_block(); | ||
| } | ||
|
|
||
| // split segments into well-aligned blocks | ||
| int idx = 0; | ||
| while (idx < _nonoop_acmp_map->length()) { | ||
| int offset = _nonoop_acmp_map->adr_at(idx)->first; | ||
| int size = _nonoop_acmp_map->adr_at(idx)->second; | ||
| int mod = offset % 8; | ||
| switch (mod) { | ||
| case 0: | ||
| break; | ||
| case 4: | ||
| split_after(_nonoop_acmp_map, idx, 4); | ||
| break; | ||
| case 2: | ||
| case 6: | ||
| split_after(_nonoop_acmp_map, idx, 2); | ||
| break; | ||
| case 1: | ||
| case 3: | ||
| case 5: | ||
| case 7: | ||
| split_after(_nonoop_acmp_map, idx, 1); | ||
| break; | ||
| default: | ||
| ShouldNotReachHere(); | ||
| } | ||
| idx++; | ||
| } | ||
| } | ||
|
|
||
| void FieldLayoutBuilder::epilogue() { | ||
| // Computing oopmaps | ||
| OopMapBlocksBuilder* nonstatic_oop_maps = | ||
|
|
@@ -1338,6 +1489,13 @@ void FieldLayoutBuilder::epilogue() { | |
| _info->_is_empty_inline_klass = _is_empty_inline_class; | ||
| } | ||
|
|
||
| // Acmp maps are needed for both concrete and abstract value classes | ||
| if (_is_inline_type || _is_abstract_value) { | ||
| _info->_acmp_maps_offset = _static_layout->acmp_maps_offset(); | ||
| _info->_nonoop_acmp_map = _nonoop_acmp_map; | ||
| _info->_oop_acmp_map = _oop_acmp_map; | ||
| } | ||
|
|
||
| // This may be too restrictive, since if all the fields fit in 64 | ||
| // bits we could make the decision to align instances of this class | ||
| // to 64-bit boundaries, and load and store them as single words. | ||
|
|
@@ -1411,6 +1569,16 @@ void FieldLayoutBuilder::epilogue() { | |
| if (_null_marker_offset != -1) { | ||
| st.print_cr("Null marker offset = %d", _null_marker_offset); | ||
| } | ||
| st.print("Non-oop acmp map: "); | ||
| for (int i = 0 ; i < _nonoop_acmp_map->length(); i++) { | ||
| st.print("<%d,%d>, ", _nonoop_acmp_map->at(i).first, _nonoop_acmp_map->at(i).second); | ||
| } | ||
| st.print_cr(""); | ||
| st.print("oop acmp map: "); | ||
| for (int i = 0 ; i < _oop_acmp_map->length(); i++) { | ||
| st.print("%d, ", _oop_acmp_map->at(i)); | ||
| } | ||
| st.print_cr(""); | ||
| } | ||
| st.print_cr("---"); | ||
| // Print output all together. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this comment. Explains why abstract classes are omitted from above.