On Wed, 14 Aug 2024 19:34:26 GMT, Leonid Mesnik <lmes...@openjdk.org> wrote:
> The summary of the problem. Test is intermittently failing because can't get > expected field watch event. > The test is failing to get event in the 'setfmodw001b' thread with Xcomp only. > I verified that frame with the method 'run' of setfmodw001b is compiled when > line > 118: setfmodw001.setWatch(4); > is executed, however the thread is in interp_only mode. The watch events are > supported by interpreter only and just ignored by compiled code. > > The reason of failure is race beteween setting interp_only mode in line > > https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001.java#L75 > > and calling method call_helper while > the method run() > https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001.java#L116 > > in newly created thread 'setfmodw001b' is invoked. > > The javaCalls:call are used to invoke methods from hotspot, so it might be > rare issues. But still, synchronization might be improved. > The > void JavaCalls::call_helper(JavaValue* result, const methodHandle& method, > JavaCallArguments* args, TRAPS) > > checks if interp_only mode is set and use 'Method::from_interpreted_entry()' > if not. However the interp_only might be set later before compiled method is > called (or enter first safe point?). This might happens in safepoint during > transition via handshake. > So the running thread is in interp_only mode however the run() method is > compiled and executed already and never going to be deoptimized. > > The additional setWatch calls don't try to deptimize method that are already > in interp_only mode. > > BTW, the when JVMCI is enabled and verified adapter exists it is also will be > loaded even in interp_only mode set. There is not race here, just ignoring of > interp_only mode. > > I run failing test with Xcomp ~1000 times and tiers1-5. src/hotspot/share/runtime/javaCalls.cpp line 403: > 401: } else { > 402: // Since the call stub sets up like the interpreter we call > the from_interpreted_entry > 403: // so we can go compiled via a i2c. Do you think removed comment "Otherwise initial entry method will always run interpreted.", or it's understood and redundant? src/hotspot/share/runtime/javaCalls.cpp line 413: > 411: // respect to nmethod sweeping. > 412: address > 413: verified_entry_point = (address) > HotSpotJVMCI::InstalledCode::entryPoint(nullptr, alternative_target()); This line-break choice seems unconventional. Can we just make it a single line like before? test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetFieldModificationWatch/setfmodw001/TestDescription.java line 57: > 55: * /test/lib > 56: * @run main/othervm/native -agentlib:setfmodw001 > -XX:TraceJVMTI=ec+,+ioe,+s -Xlog:jvmti=trace:file=vm.%p.log > nsk.jvmti.SetFieldModificationWatch.setfmodw001 > 57: */ What's the reason for removing this test run? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20587#discussion_r1723692429 PR Review Comment: https://git.openjdk.org/jdk/pull/20587#discussion_r1723691524 PR Review Comment: https://git.openjdk.org/jdk/pull/20587#discussion_r1723690592
