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

[Xiaohong Gong]

On Wed, 23 Aug 2023 07:39:24 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

>>> > > Are you inclined to re-instantiate the old behavior ?
>>> > 
>>> > 
>>> > May I ask what the "old behavior" do you mean? Thanks!
>>> 
>>> Hi @XiaohongGong , Updated my previous comment, I am able reproduce an intermittent crash in another test "compiler/valhalla/inlinetypes/TestGetfieldChains.java" with additional JVM options : "-Xbatch -XX:TieredStopAtLevel=1 -XX:CompileThresholdScaling=0.1 -XX:InlineFieldMaxFlatSize=0" [hs_err_pid1570004.txt](https://github.com/openjdk/valhalla/files/12385038/hs_err_pid1570004.txt) [replay_pid1570004.txt](https://github.com/openjdk/valhalla/files/12385039/replay_pid1570004.txt) [TestGetfieldChains.txt](https://github.com/openjdk/valhalla/files/12385040/TestGetfieldChains.txt)
>>> 
>>> Please find attached relevant logs and replay file.
>>> 
>>> Best Regards, Jatin
>> 
>> Thanks for pointing out this! I can reproduce this issue after I rebase my patch to latest `lworld` branch. I will try my best to look at what is changed. 
>> 
>> Best Regards,
>> Xiaohong
>
> Hi @XiaohongGong,
> 
>> I think there maybe some potential issues exposed after this change, since almost all value objects cannot be flattened anymore with -XX:InlineFieldMaxFlatSize=0, which may need the buffer allocated.
> 
> Yes, I think it's likely that this change reveals some existing issues. I can help with debugging but only later next month.
> 
>> Could you please show more env/options information on this issue? I cannot reproduce it even with -XX:InlineFieldMaxFlatSize=0 -XX:TieredStopAtLevel=1 on our internal Arm NEON machine.
> 
> It fails quite reliable in our testing on x86_64 with one of the following flag combinations:
> - `-Xcomp -XX:TieredStopAtLevel=1 -DIgnoreCompilerControls=true`
> - `-Xcomp -XX:-TieredCompilation -DIgnoreCompilerControls=true`
> - `-DWarmup=0 -DVerifyIR=false`
> - `-DWarmup=0 -XX:TieredStopAtLevel=1`
> 
> But you can reproduce this now, right?
> 
>> Actually I don't quite understand what this assertion mean? Why the null_free value type must be allocated in this routine?
> 
> The `is_allocated` assertion checks that the InlineTypeNode should always have a valid oop to a heap buffer now that we just loaded it from an oop value. That's not guaranteed if the field/argument can be NULL though, that's why the assert checks for `!null_free`. Does that make sense?

Hi @TobiHartmann,


# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error (/workspace/open/src/hotspot/share/c1/c1_LIRGenerator.cpp:1778), pid=336001, tid=336015
#  assert(field->type()->as_inline_klass()->is_initialized()) failed: Must be
#
# JRE version: Java(TM) SE Runtime Environment (22.0) (fastdebug build 22-lworld4ea-2023-08-16-1408411.tobias.hartmann.valhalla2)
# Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug 22-lworld4ea-2023-08-16-1408411.tobias.hartmann.valhalla2, compiled mode, emulated-client, sharing, tiered, compressed oops, compressed class ptrs, g1 gc, linux-amd64)
# Problematic frame:
# V  [libjvm.so+0x7dc168]  LIRGenerator::access_sub_element(LIRItem&, LIRItem&, LIR_Opr&, ciField*, int)+0x808

Current CompileTask:
C1:   7349 3404    b  1       compiler.valhalla.inlinetypes.TestC1::test7_verifier (47 bytes)


Recently I spent sometime looking at this crash when the field is not flattened. The basic conclusion is that I didn't find out the field's flatten status can influence the klass's initialization. As a further investigation for this case, I found the code path is different in C1 compiler when the field is flattened or not.  

Here is my understanding to the process:

The relative ops are a flattened array element loading and a followed field loading from the array element. The array element and the field of the element are both primitive class type. To optimize the whole process, C1 compiler merges the two access ops into one with a delay field access. This saves the object re-materialization from the flattened style during array access. 

When the field is not flattened, it goes to method `LIRGenerator::access_sub_element()`. And the assertion is added after the primitive field is loaded. C1 compiler will set the value to the primitive class's default value if the loaded field is null. And the default value requires the corresponding primitive klass is initialized. 

And when the field is flattened, it goes to method `LIRGenerator::access_flattened_array()`. Before it, an oop buffer is allocated, and in this method, it directly fills the fields information to the allocated oop buffer. The whole process doesn't need the klass's fully initialized since the default value is not used.  To prove my assumption that field's flattened status will not influence the klass's initialization, I added the same assertion in this method (after https://github.com/openjdk/valhalla/blob/lworld/src/hotspot/share/c1/c1_LIRGenerator.cpp#L1804). And it can fail sometimes as well.

As a summary, I think we'd better check the klass's initialization state before the whole process in C1 if it is necessary, just like:

diff --git a/src/hotspot/share/c1/c1_GraphBuilder.cpp b/src/hotspot/share/c1/c1_GraphBuilder.cpp
index 78cfd1796..35e408f34 100644
--- a/src/hotspot/share/c1/c1_GraphBuilder.cpp
+++ b/src/hotspot/share/c1/c1_GraphBuilder.cpp
@@ -1123,7 +1123,7 @@ void GraphBuilder::load_indexed(BasicType type) {
     if (s.cur_bc() == Bytecodes::_getfield) {
       bool will_link;
       ciField* next_field = s.get_field(will_link);
-      bool next_needs_patching = !next_field->holder()->is_loaded() ||
+      bool next_needs_patching = !next_field->holder()->is_initialized() ||
                                  !next_field->will_link(method(), Bytecodes::_getfield) ||
                                  PatchALot;
       can_delay_access = C1UseDelayedFlattenedFieldReads && !next_needs_patching; 

Any better idea to fixing this issue? Please correct me if any misunderstanding! Thanks a lot!

Best Regards,
Xiaohong

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

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