[lworld] RFR: 8311219: [lworld] VM option "InlineFieldMaxFlatSize" cannot work well [v3]

[Tobias Hartmann]

On Tue, 5 Sep 2023 02:50:22 GMT, Xiaohong Gong <xgong at openjdk.org> wrote:

>> Currently all the non-static final fields with inline type can be flattened, even if the layout size of the inline type is beyond
>> the max flat size specified by `InlineFieldMaxFlatSize`. Please refer to the condition check [1] which decides whether a field
>> can be flattened or not.
>> 
>> Field flattening has two major side effects: atomicity and size. Fields with atomic access limitation or large size that exceeds
>> the specified threshold value cannot be flattened. And final fields are special that they are immutable after initialized. So the atomic check for them can be ignored. Hence, 1) for the atomicity free type like the primitive class, the final and non-final fields with such type can be flattened. And 2) for the normal value class that has atomic feature, only the final fields with such type can be flattened. And all kinds of the flattened fields should not exceed the specified max flat size. Please see more details from [1] [2].
>> 
>> The original condition [1] matches the atomicity check but not the flat size limitation. Promoting the flat size check before all other checks matches the flattening policy and can make the VM option `InlineFieldMaxFlatSize` work for final fields as well.
>> 
>> This patch also fixed the jtreg crashes involved after the field flattening condition is changed. Those tests fail with setting
>> `-XX:+InlineFieldMaxFlatSize=0` by default. The main issue is the non-flattened inline type field is not buffered which is expected to be. The root cause is when parsing `withfield`, the compiler checks whether the field is primitive class type while not its flattening status. Changing to check the flattening status instead can fix the crash.
>> 
>> [1] https://github.com/openjdk/valhalla/blob/lworld/src/hotspot/share/classfile/fieldLayoutBuilder.cpp#L759
>> [2] https://mail.openjdk.org/pipermail/valhalla-dev/2023-June/011262.html
>> [3] https://mail.openjdk.org/pipermail/valhalla-dev/2023-July/011265.html
>
> Xiaohong Gong has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix "assert(field->type()->as_inline_klass()->is_initialized()) failed: Must be"

Yes, please include it in this PR but on second thought, the proper fix should look like this:


-  assert(!null_free || vt->is_allocated(&gvn), "inline type should be allocated");
+  assert(vt->is_allocated(&gvn) || (null_free && !vk->is_initialized()), "inline type should be allocated");


All InlineTypeNodes returned by this method should be allocated except for null free with uninitialized value class (because we can't load the default oop for those).

Testing found some additional issues triggered by your change. Please include below fixes as well.

We hit the `"Should have been buffered"` assert because the verification code is too strong. InlineType users don't require buffering because they are removed as well. In addition, the `u->Opcode() != Op_Return || !tf()->returns_inline_type_as_fields()` condition is not required.


--- a/src/hotspot/share/opto/compile.cpp
+++ b/src/hotspot/share/opto/compile.cpp
@@ -2045,16 +2045,10 @@ void Compile::process_inline_types(PhaseIterGVN &igvn, bool remove) {
 #ifdef ASSERT
         // Verify that inline type is buffered when replacing by oop
         else if (u->is_InlineType()) {
-          InlineTypeNode* vt2 = u->as_InlineType();
-          for (uint i = 0; i < vt2->field_count(); ++i) {
-            if (vt2->field_value(i) == vt && !vt2->field_is_flattened(i)) {
-              // Use in non-flat field
-              must_be_buffered = true;
-            }
-          }
+          // InlineType uses don't need buffering because they are about to be replaced as well
         } else if (u->is_Phi()) {
           // TODO 8302217 Remove this once InlineTypeNodes are reliably pushed through
-        } else if (u->Opcode() != Op_Return || !tf()->returns_inline_type_as_fields()) {
+        } else {
           must_be_buffered = true;
         }
         if (must_be_buffered && !vt->is_allocated(&igvn)) {


When flattening is disabled, we sometimes bail out from compiling `test51` due to `COMPILE SKIPPED: unsupported calling sequence`. That's expected because the call to the `test` method already occupies a lot of stack space. I adjusted the test:


diff --git a/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestLWorld.java b/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestLWorld.java
index e1ecca66bd0..d34feba1c48 100644
--- a/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestLWorld.java
+++ b/test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestLWorld.java
@@ -1609,10 +1609,15 @@ public class TestLWorld {
         }
     }
 
+    // Pass arguments via fields to avoid exzessive spilling leading to compilation bailouts
+    static Test51Value test51_arg1;
+    static MyValue1 test51_arg2;
+    static Object test51_arg3;
+
     // Same as test2 but with field holder being an inline type
     @Test
-    public long test51(Test51Value holder, MyValue1 vt1, Object vt2) {
-        return holder.test(holder, vt1, vt2);
+    public long test51() {
+        return test51_arg1.test(test51_arg1, test51_arg2, test51_arg3);
     }
 
     @Run(test = "test51")
@@ -1622,7 +1627,10 @@ public class TestLWorld {
         Test51Value holder = new Test51Value();
         Asserts.assertEQ(testValue1.hash(), vt.hash());
         Asserts.assertEQ(holder.valueField1.hash(), vt.hash());
-        long result = test51(holder, vt, vt);
+        test51_arg1 = holder;
+        test51_arg2 = vt;
+        test51_arg3 = vt;
+        long result = test51();
         Asserts.assertEQ(result, 9*vt.hash() + def.hashPrimitive());
     }

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

PR Comment: https://git.openjdk.org/valhalla/pull/888#issuecomment-1708138308