On Tue, 6 Aug 2024 17:26:55 GMT, Jorn Vernee <[email protected]> wrote:
> As discussed in the JBS issue:
>
> FFM upcall stubs embed a `Method*` of the target method in the stub. This
> `Method*` is read from the `LambdaForm::vmentry` field associated with the
> target method handle at the time when the upcall stub is generated. The MH
> instance itself is stashed in a global JNI ref. So, there should be a
> reachability chain to the holder class: `MH (receiver) -> LF (form) ->
> MemberName (vmentry) -> ResolvedMethodName (method) -> Class<?> (vmholder)`
>
> However, it appears that, due to multiple threads racing to initialize the
> `vmentry` field of the `LambdaForm` of the target method handle of an upcall
> stub, it is possible that the `vmentry` is updated _after_ we embed the
> corresponding `Method`* into an upcall stub (or rather, the latest update is
> not visible to the thread generating the upcall stub). Technically, it is
> fine to keep using a 'stale' `vmentry`, but the problem is that now the
> reachability chain is broken, since the upcall stub only extracts the target
> `Method*`, and doesn't keep the stale `vmentry` reachable. The holder class
> can then be unloaded, resulting in a crash.
>
> The fix I've chosen for this is to mimic what we already do in
> `MethodHandles::jump_to_lambda_form`, and re-load the `vmentry` field from
> the target method handle each time. Luckily, this does not really seem to
> impact performance.
>
> <details>
> <summary>Performance numbers</summary>
> x64:
>
> before:
>
> Benchmark Mode Cnt Score Error Units
> Upcalls.panama_blank avgt 30 69.216 ± 1.791 ns/op
>
>
> after:
>
> Benchmark Mode Cnt Score Error Units
> Upcalls.panama_blank avgt 30 67.787 ± 0.684 ns/op
>
>
> aarch64:
>
> before:
>
> Benchmark Mode Cnt Score Error Units
> Upcalls.panama_blank avgt 30 61.574 ± 0.801 ns/op
>
>
> after:
>
> Benchmark Mode Cnt Score Error Units
> Upcalls.panama_blank avgt 30 61.218 ± 0.554 ns/op
>
> </details>
>
> As for the added TestUpcallStress test, it takes about 800 seconds to run
> this test on the dev machine I'm using, so I've set the timeout quite high.
> Since it runs for so long, I've dropped it from the default `jdk_foreign`
> test suite, which runs in tier2. Instead the new test will run in tier4.
>
> Testing: tier 1-4
RuntimeAddress will not work for s390x. @JornVernee would you please apply
these changes.
Thanks @TheRealMDoerr for pointing it out. I will create a JBS issue for
implementing `resolve_global_jobject`.
diff --git a/src/hotspot/cpu/s390/upcallLinker_s390.cpp
b/src/hotspot/cpu/s390/upcallLinker_s390.cpp
index 1b07522858f..8baad40a519 100644
--- a/src/hotspot/cpu/s390/upcallLinker_s390.cpp
+++ b/src/hotspot/cpu/s390/upcallLinker_s390.cpp
@@ -216,10 +216,11 @@ address UpcallLinker::make_upcall_stub(jobject receiver,
Symbol* signature,
arg_shuffle.generate(_masm, shuffle_reg, abi._shadow_space_bytes,
frame::z_jit_out_preserve_size);
__ block_comment("} argument_shuffle");
- __ block_comment("load target {");
+ __ block_comment("load_target {");
__ load_const_optimized(Z_ARG1, (intptr_t)receiver);
- __ call(RuntimeAddress(StubRoutines::upcall_stub_load_target())); // load
taget Method* into Z_method
- __ block_comment("} load target");
+ __ load_const_optimized(call_target_address,
StubRoutines::upcall_stub_load_target());
+ __ call(call_target_address); // load taget Method* into Z_method
+ __ block_comment("} load_target");
__ z_lg(call_target_address, Address(Z_method,
in_bytes(Method::from_compiled_offset())));
__ call(call_target_address);
-------------
Changes requested by amitkumar (Committer).
PR Review: https://git.openjdk.org/jdk/pull/20479#pullrequestreview-2276394431
