RFR: 8032218: Emit single post-constructor barrier for chain of superclass constructors [v4]

Tobias Hartmann thartmann at openjdk.org
Mon Aug 26 06:29:07 UTC 2024 

On Sun, 18 Aug 2024 07:35:41 GMT, Joshua Cao <duke at openjdk.org> wrote:

>> [C2 emits a StoreStore barrier for each constructor call](https://github.com/openjdk/jdk/blob/72ca7bafcd49a98c1fe09da72e4e47683f052e9d/src/hotspot/share/opto/parse1.cpp#L1016) in a chain of superclass constructor calls. It is unnecessary. We only need to emit a single barrier for each object allocation / each pair of `Allocation/InitializeNode`. 
>> 
>> [Macro expansion emits a trailing StoreStore after an InitializeNode](https://github.com/openjdk/jdk/blob/32946e1882e9b22c983cbba3c6bda3cc7295946a/src/hotspot/share/opto/macro.cpp#L1610-L1628). This `StoreStore` is sufficient as the post-constructor barrier. From the [InitializeNode definition](https://github.com/openjdk/jdk/blob/32946e1882e9b22c983cbba3c6bda3cc7295946a/src/hotspot/share/opto/memnode.cpp#L3639-L3642):
>> 
>>> // An InitializeNode collects and isolates object initialization after
>> // an AllocateNode and before the next possible safepoint.  As a
>> // memory barrier (MemBarNode), it keeps critical stores from drifting
>> // down past any safepoint or any publication of the allocation.
>> 
>> This PR modifies `Parse::do_exits()` such that it only emits a barrier for a constructor if we find that the constructed object does not have an `InitializeNode`. It is possible that we cannot find an `InitializeNode` i.e. if the outermost method of the compilation unit is the constructor. We still need to emit a barrier in these cases.
>> 
>> Passes hotspot tier1 locally on x86 linux machine. New tests make sure that there is a single `StoreStore` for chained constructors.
>
> Joshua Cao has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 27 commits:
> 
>  - Add tests for Stable fields
>  - Fix typo in comment block
>  - Merge branch 'master' into chainstorestore
>  - Attempt2: Only emit StoreStore in do_exits when there is no parent
>    caller
>  - Merge branch 'master' of https://git.openjdk.org/jdk into chainstorestore
>  - 8032218: Emit single post-constructor barrier for chain of superclass constructors
>  - Add riscv64 to test
>  - Merge branch 'master' into storestore
>  - Merge branch 'master' into storestore
>  - Apply suggestions from code review
>    
>    some formatting suggestions from @shipilev
>    
>    Co-authored-by: Aleksey Shipilëv <shipilev at amazon.de>
>  - ... and 17 more: https://git.openjdk.org/jdk/compare/8635642d...acca7a26

`compiler/stringopts/TestStringObjectInitialization.java` fails on Linux AArch64 with `-XX:-TieredCompilation -XX:+AlwaysIncrementalInline`:


java.lang.NullPointerException: Cannot read the array length because "this.value" is null
	at java.base/java.lang.String.length(String.java:1593)
	at java.base/java.lang.AbstractStringBuilder.append(AbstractStringBuilder.java:590)
	at java.base/java.lang.StringBuilder.append(StringBuilder.java:179)
	at compiler.stringopts.TestStringObjectInitialization.add(TestStringObjectInitialization.java:62)
	at compiler.stringopts.TestStringObjectInitialization.run(TestStringObjectInitialization.java:67)
	at compiler.stringopts.TestStringObjectInitialization$Runner.run(TestStringObjectInitialization.java:85)
	at java.base/java.lang.Thread.run(Thread.java:1575)
STATUS:Failed.`main' threw exception: java.lang.NullPointerException: Cannot read the array length because "this.value" is null
java.lang.NullPointerException: Cannot read the array length because "this.value" is null
	at java.base/java.lang.String.length(String.java:1593)
	at java.base/java.lang.AbstractStringBuilder.append(AbstractStringBuilder.java:590)
	at java.base/java.lang.StringBuilder.append(StringBuilder.java:179)
	at compiler.stringopts.TestStringObjectInitialization.add(TestStringObjectInitialization.java:62)
	at compiler.stringopts.TestStringObjectInitialization.run(TestStringObjectInitialization.java:67)
	at compiler.stringopts.TestStringObjectInitialization$Runner.run(TestStringObjectInitialization.java:85)
	at java.base/java.lang.Thread.run(Thread.java:1575)
STATUS:Failed.`main' threw exception: java.lang.NullPointerException: Cannot read the array length because "this.value" is null
java.lang.NullPointerException: Cannot read the array length because "this.value" is null
	at java.base/java.lang.String.length(String.java:1593)
	at java.base/java.lang.AbstractStringBuilder.append(AbstractStringBuilder.java:590)
	at java.base/java.lang.StringBuilder.append(StringBuilder.java:179)
	at compiler.stringopts.TestStringObjectInitialization.add(TestStringObjectInitialization.java:62)
	at compiler.stringopts.TestStringObjectInitialization.run(TestStringObjectInitialization.java:67)
	at compiler.stringopts.TestStringObjectInitialization$Runner.run(TestStringObjectInitialization.java:85)
	at java.base/java.lang.Thread.run(Thread.java:1575)


I also see this with an internal stress test:


# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error (/workspace/open/src/hotspot/share/opto/escape.cpp:4674), pid=22930, tid=42355
#  assert(n->is_Mem()) failed: memory node required.
#
# JRE version: Java(TM) SE Runtime Environment (24.0) (fastdebug build 24-internal-2024-08-26-0524112.tobias.hartmann.jdk3)
# Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug 24-internal-2024-08-26-0524112.tobias.hartmann.jdk3, mixed mode, tiered, compressed class ptrs, z gc, linux-amd64)
# Problematic frame:
# V  [libjvm.so+0xbcc316]  ConnectionGraph::split_unique_types(GrowableArray<Node*>&, GrowableArray<ArrayCopyNode*>&, GrowableArray<MergeMemNode*>&, Unique_Node_List&)+0x3796
#


Current CompileTask:
C2:617119 124774       4       sun.util.locale.LocaleExtensions::toID (136 bytes)

Stack: [0x00007fd262f9b000,0x00007fd26309b000],  sp=0x00007fd263095a70,  free space=1002k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V  [libjvm.so+0xbcc316]  ConnectionGraph::split_unique_types(GrowableArray<Node*>&, GrowableArray<ArrayCopyNode*>&, GrowableArray<MergeMemNode*>&, Unique_Node_List&)+0x3796  (escape.cpp:4674)
V  [libjvm.so+0xbd46b9]  ConnectionGraph::compute_escape()+0x20e9  (escape.cpp:397)
V  [libjvm.so+0xbd4f11]  ConnectionGraph::do_analysis(Compile*, PhaseIterGVN*)+0xf1  (escape.cpp:119)
V  [libjvm.so+0x9fbeaa]  Compile::Optimize()+0x63a  (compile.cpp:2324)
V  [libjvm.so+0x9ffe13]  Compile::Compile(ciEnv*, ciMethod*, int, Options, DirectiveSet*)+0x1b43  (compile.cpp:852)
V  [libjvm.so+0x84f2c5]  C2Compiler::compile_method(ciEnv*, ciMethod*, int, bool, DirectiveSet*)+0x1d5  (c2compiler.cpp:142)
V  [libjvm.so+0xa0bad8]  CompileBroker::invoke_compiler_on_method(CompileTask*)+0x928  (compileBroker.cpp:2303)
V  [libjvm.so+0xa0c768]  CompileBroker::compiler_thread_loop()+0x478  (compileBroker.cpp:1961)
V  [libjvm.so+0xeb67bc]  JavaThread::thread_main_inner()+0xcc  (javaThread.cpp:758)
V  [libjvm.so+0x17e2ab6]  Thread::call_run()+0xb6  (thread.cpp:225)
V  [libjvm.so+0x14cc747]  thread_native_entry(Thread*)+0x127  (os_linux.cpp:858)

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

PR Comment: https://git.openjdk.org/jdk/pull/18870#issuecomment-2309423452

More information about the hotspot-compiler-dev mailing list