[lworld] RFR: 8370450: [lworld] Alternate implementation of the substitutability test method [v2]

[Coleen Phillimore]

On Thu, 23 Oct 2025 20:52:24 GMT, Frederic Parain <fparain at openjdk.org> wrote:

>> src/hotspot/share/classfile/fieldLayoutBuilder.cpp line 1361:
>> 
>>> 1359:   _nonoop_acmp_map = new GrowableArray<Pair<int,int>>();
>>> 1360:   _oop_acmp_map = new GrowableArray<int>();
>>> 1361:   if (_is_empty_inline_class) return;
>> 
>> Why not return first before allocating these GrowableArray in resource area?
>
> It streamlines the code in InstanceKlass::fill_instance_klass() by not having to handle the null map case.

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.

>> src/hotspot/share/classfile/fieldLayoutBuilder.cpp line 1381:
>> 
>>> 1379:       case LayoutRawBlock::REGULAR:
>>> 1380:         {
>>> 1381:           FieldInfo* fi = _field_info->adr_at(b->field_index());
>> 
>> Is there an assumption that the block field indexes are in ascending order?
>
> I'm not sure I understand the question.
> LayoutRawBlocks represent the fields layout, they are chained in ascending offset order.
> The field_index is the index in the FieldInfo array (from the class file) where this field is declared.

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.

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/1695#discussion_r2461400686
PR Review Comment: https://git.openjdk.org/valhalla/pull/1695#discussion_r2461413743