RFR: 8337753: Target class of upcall stub may be unloaded

[Jorn Vernee]

On Tue, 6 Aug 2024 17:26:55 GMT, Jorn Vernee <jvernee at openjdk.org> 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

I had to adjust the stub size on x64 when running on fastdebug/shenandoah. That may also be needed on other platforms except for aarch64), but I can't test there.

To find the right stub size, just increase `upcall_stub_code_base_size` to a high number (e.g. 2048), and then run this program:


import java.lang.foreign.*;
import java.lang.invoke.*;

public class Main {
    public static void main(String[] args) throws Throwable {
        try (Arena arena = Arena.ofConfined()) {
            MemorySegment stub = Linker.nativeLinker().upcallStub(
                MethodHandles.empty(MethodType.methodType(void.class)),
                FunctionDescriptor.ofVoid(),
                arena);
        }   
    }
}



$ javac -d classes Main.java
$ java -cp classes -XX:+UseShenandoahGC -XX:+LogCompilation Main
$ grep upcall_stub -A 1 hotspot_pid*
<blob name='upcall_stub_()V' total_size='1360'>
<sect index='1' capacity='1360' size='1195' remaining='165'/>


Then set `upcall_stub_code_base_size` to be bigger than `size`.

Using a static stub is still a bit slower, but much more in line with the performance of inline loads:


Current:

Benchmark                  Mode  Cnt    Score   Error  Units
QSort.panama_upcall_qsort  avgt   30  608.953 � 3.047  ns/op


Fully C++:

Benchmark                  Mode  Cnt    Score   Error  Units
QSort.panama_upcall_qsort  avgt   30  725.142 � 2.718  ns/op   ~19% slower


Static stub:

Benchmark                  Mode  Cnt    Score   Error  Units
QSort.panama_upcall_qsort  avgt   30  627.661 � 2.099  ns/op   ~3% slower


I think that's a good compromise. (Although I wish the C++ code was just as fast, as it's much nicer)

> Some of the DecoratorSet should be applicable and improve performance.

I gave `AS_NO_KEEPALIVE` a try. I'm not sure if that's correct, but it didn't really change performance. I'm not sure what other decorator would apply. I was looking at `ACCESS_READ`, but it seems that that can not be used for these loads.

I've added the implementations with the stubs, I had to eyeball `s390, ppc, and rsicv, so testing is still needed on those platforms (GHA will do the cross builds).

I also spent quite a while messing with the test. This test is quite unstable, because it creates a new thread pool for each test case, and then calls `ExecutorService::shutdownNow`, which doesn't allow submitted tasks to finish. It was running out of memory on some of the mac machine we test on in CI. I've made several attempts to make it more stable, but all of those resulted in the issue no longer being reproduced. For now I've restricted the test to linux/aarch64, since that's where we see the issue, and it seems stable enough to pass every time at least in out CI. If it causes issues though, I think we might have to just drop the test, or maybe mark it as `/manual`, so it doesn't run in CI.

I'll be away until September. Will pick this back up then.

With the latest version, we are slightly faster than the baseline:


baseline:

Benchmark                  Mode  Cnt    Score   Error  Units
QSort.panama_upcall_qsort  avgt   30  635.047 � 2.181  ns/op

Patched:

Benchmark                  Mode  Cnt    Score   Error  Units
QSort.panama_upcall_qsort  avgt   30  625.385 � 2.442  ns/op


@TheRealMDoerr Thanks for all the suggestions!

src/hotspot/cpu/x86/upcallLinker_x86_64.cpp line 311:

> 309:                     Address(rbx, java_lang_invoke_ResolvedMethodName::vmtarget_offset()),
> 310:                     noreg, noreg);
> 311:   __ movptr(Address(r15_thread, JavaThread::callee_target_offset()), rbx); // just in case callee is deoptimized

FWIW, I tried to move this code to `UpcallLinker::on_entry`, but there is a speed cost to that (of around 10ns on my machine). Doing that results in several out-of-line calls to `AccessInternal::RuntimeDispatch<...>::_load_at_func` to load the values. It seems like a tradeoff between speed and space (in the code cache). I went with speed here.

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

PR Comment: https://git.openjdk.org/jdk/pull/20479#issuecomment-2273455863
PR Comment: https://git.openjdk.org/jdk/pull/20479#issuecomment-2277864015
PR Comment: https://git.openjdk.org/jdk/pull/20479#issuecomment-2278175462
PR Comment: https://git.openjdk.org/jdk/pull/20479#issuecomment-2278557487
PR Comment: https://git.openjdk.org/jdk/pull/20479#issuecomment-2326568409
PR Review Comment: https://git.openjdk.org/jdk/pull/20479#discussion_r1706122881