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

[Tobias Hartmann]

On Mon, 4 Sep 2023 04:08:20 GMT, Xiaohong Gong <xgong at openjdk.org> wrote:

>> 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/...

Great analysis @XiaohongGong! The fix looks good to me.

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

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