Hi Coleen,
On 14/04/2020 12:34 am, coleen.phillim...@oracle.com wrote:
Summary: Do not install async exceptions at_safepoint for each bytecode.
I'm still not certain that we have to go this far to solve this problem,
but it does sound like a relatively simple solution provided there are
no unintended consequences.
See CR for a lot more details. This change calls a new
InterpreterRuntime::at_safepoint_async_safe() which installs the async
exception in the interpreter at backward branches and returns. This
uses safepoint polling code in the interpreter for each platform. These
changes (cross) compile on platforms that Oracle doesn't support but I
don't know if they work.
I'm not convinced the platform specific changes are necessary, because
calls to the runtime from many bytecodes will install the async
exception, so it's essentially installed "enough" for this deprecated
feature. I tested the changes with *and* without the platform specific
changes with no failure, which included the jdb, jdi and jvmti
serviceability tests.
I don't understand what you mean here. If the whole basis of this fix is
"don't install async exceptions other than at backward branches and
returns" then how is that implemented without the changes in the
interpreter code?
If this can be fixed just by adjusting the actual monitor code then I
would much prefer that. It took me a while to get my head around the
dispatch changes in interpreter code and even then I don't see how those
changes only impact backward branches and returns ??
This change also makes InterpreterRuntime::monitorexit a JRT_LEAF
bytecode. The code to check for exceptions is outside the runtime call.
I ran the JCK vm and lang tests on this change with no failure.
Tested with tier1-6.
open webrev at http://cr.openjdk.java.net/~coleenp/2020/8074292.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8074292
./cpu/x86/interp_masm_x86.cpp
It took me a long time to figure out how the new logic worked compared
to the old logic. Even then I'm unclear about the effective recursive
dispatch path: dispatch_base(generate_poll=true) -> dispatch_via ->
dispatch_base(generate_poll=false) - does it work okay with
VerifyActivationFrameSize? It seems a rather convoluted way to
effectively just execute:
858 lea(rscratch1, ExternalAddress((address)table));
859 jmp(Address(rscratch1, rbx, Address::times_8));
---
src/hotspot/share/interpreter/interpreterRuntime.cpp
How were you able to drop this code:
791 if (elem == NULL || h_obj()->is_unlocked()) {
792 THROW(vmSymbols::java_lang_IllegalMonitorStateException());
793 }
?
and this:
798 #ifdef ASSERT
799 thread->last_frame().interpreter_frame_verify_monitor(elem);
800 #endif
?
Thanks,
David
Thanks,
Coleen
