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

[Coleen Phillimore]

On Wed, 22 Oct 2025 18:17:30 GMT, Frederic Parain <fparain at openjdk.org> wrote:

>> This is an alternate version of the substitutability method.
>> To use it, add -Xshare:off -XX:+UseAltSubstitutabilityMethod to the command line of the Valhalla VM in preview mode.
>
> Frederic Parain has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits:
> 
>  - Merge remote-tracking branch 'upstream/lworld' into new_acmp
>  - Add alternate version of the substitutability method

It's really great work.  I have some questions and minor suggested changes.

src/hotspot/share/classfile/classFileParser.cpp line 1581:

> 1579:   if (!access_flags().is_identity_class() && !access_flags().is_interface()
> 1580:       && _class_name != vmSymbols::java_lang_Object()) {
> 1581:     // Acmp map required for abstract and concrete value classes

Thank you for this comment.  Explains why abstract classes are omitted from above.

src/hotspot/share/classfile/classFileParser.cpp line 5546:

> 5544:     // Current format of acmp maps:
> 5545:     // All maps are stored contiguously in a single int array because it might
> 5546:     // be to early to instantiate an Object array (to be investigated)

Suggestion:

    // be to early too instantiate an Object array (to be investigated)

src/hotspot/share/classfile/fieldLayoutBuilder.cpp line 1303:

> 1301:   int start = map->adr_at(last_idx)->first > offset ? 0 : last_idx;
> 1302:   bool inserted = false;
> 1303:   for (int c = start; c < map->length() && !inserted ; c++) {

You don't need to check inserted in this for loop because you have breaks, if I'm reading this right.

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?

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?

src/hotspot/share/interpreter/bytecodeTracer.cpp line 95:

> 93:   void trace(const methodHandle& method, address bcp, uintptr_t tos, uintptr_t tos2, outputStream* st) {
> 94:     ResourceMark rm;
> 95:     ttyLocker ttyl;

This should be removed with [JDK-8370044](https://bugs.openjdk.org/browse/JDK-8370044)

src/hotspot/share/prims/unsafe.cpp line 970:

> 968: 
> 969:   if (!k->is_inline_klass()) {
> 970:         THROW_MSG_NULL(vmSymbols::java_lang_IllegalArgumentException(), "Argument is not a concrete value class");

Suggestion:

    THROW_MSG_NULL(vmSymbols::java_lang_IllegalArgumentException(), "Argument is not a concrete value class");

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

PR Review: https://git.openjdk.org/valhalla/pull/1695#pullrequestreview-3371719787
PR Review Comment: https://git.openjdk.org/valhalla/pull/1695#discussion_r2456541098
PR Review Comment: https://git.openjdk.org/valhalla/pull/1695#discussion_r2456520373
PR Review Comment: https://git.openjdk.org/valhalla/pull/1695#discussion_r2457181896
PR Review Comment: https://git.openjdk.org/valhalla/pull/1695#discussion_r2456569995
PR Review Comment: https://git.openjdk.org/valhalla/pull/1695#discussion_r2457185262
PR Review Comment: https://git.openjdk.org/valhalla/pull/1695#discussion_r2456605058
PR Review Comment: https://git.openjdk.org/valhalla/pull/1695#discussion_r2456615932