On 7/23/18 5:22 PM, serguei.spit...@oracle.com wrote:
Hi Chris,
On 7/23/18 11:40, Chris Plummer wrote:
Hi Serguei,
If the fix was complicated I would agree, but it really just boils
down to this one line change:
- fire = -1;
+ fire = 0; // Ignore this compilation. Wait for next one.
It is not obvious that this will completely fix the problem.
Is it possible that there will not be next compilation with the -Xcomp?
It's only one method that we check for. I don't see why there would be
2nd -Xcomp compilation for it, but even if there was, the test will
ignore it just like the first one. It will ignore compilations of the
method until the flag has been set indicating the method has been
executed once. If for some reason the method is never compiled after
being executed once, the test will give up waiting for it (I think after
30 seconds) and produce an error.
If it is possible then it is better to explicitly exclude these tests
for -Xcomp.
Otherwise, consider this reviewed.
Given that, I see no reason not to increase our test coverage by
supporting this test during -Xcomp runs.
I'd agree if it is going to be stable.
If problems turn up in the future, we can reconsider disabling it.
thanks,
Chris
Thanks,
Serguei
thanks,
Chris
On 7/23/18 9:44 AM, serguei.spit...@oracle.com wrote:
Hi Chris,
Would it be more simple to avoid running these tests with -Xcomp?
I guess, this would work: @requires vm.compMode != "Xcomp"
Thanks,
Serguei
On 7/23/18 00:42, Chris Plummer wrote:
Hello,
Please review the following fix for JDK11:
https://bugs.openjdk.java.net/browse/JDK-8151259
http://cr.openjdk.java.net/~cjplummer/8151259/webrev.00
It fixes the following 3 tests:
vmTestbase/nsk/jvmti/RedefineClasses/redefclass028.java
vmTestbase/nsk/jvmti/RedefineClasses/redefclass029.java
vmTestbase/nsk/jvmti/RedefineClasses/redefclass030.java
Any of which could fail when run with -Xcomp with (followed by a
bunch more errors):
# ERROR: Redefinition not started. Maybe running with -Xcomp. Test
ignored.
Although lately we've only seen this with redefclass030.java on
macosx.
These 3 tests do redefinition of a "hot" method after triggering
compilation for it. After the redef some testing is done to ensure
that the redef was done correctly, but the issue these test have
actually comes before any redef is done.
The test attempts to trigger compilation by calling a hot method a
lot. The agent detects compilation by receiving a
CompiledMethodLoad event. There was an issue discovered long ago
that when -Xcomp is used, the compilation happens before the "hot"
method is ever called. Then the redef would happen before
compilation, and this somehow messed up the test (I'm not exactly
sure how). The fix was to basically abandon the redef attempt when
this problem is detected, and then supposedly just let the test run
to completion (skipping the actual testing of the redef). After
this change, if you ran with -Xcomp it would pass, but if you
looked in the log you would see:
# ERROR: Redefinition not started. Maybe running with -Xcomp. Test
ignored.
However, there was a bug in the logic to make the test run to
completion, and also causes the above message to not appear.
Instead the test would fail with:
# ERROR: Redefinition not completed.
Followed by a bunch more error message during the part of the test
that checks if the redef was done properly.
If the CompiledMethodLoad event comes in before the hot method is
ever called (which it does with -Xcomp), the test sets fire = -1.
If the hot method was called, it is set to 1. The setting of fire
= -1 was added to fix the -Xcomp problem mentioned above. The jvmti
agent does the following:
do {
THREAD_sleep(1);
/* wait for compilation to happen */
} while(fire == 0);
if (fire == 1) {
/* do the redef here */
NSK_DISPLAY0("agentProc: <<<<<<<< RedefineClasses() is
successfully done\n");
} else {
// fire == -1
NSK_DISPLAY0("agentProc: \"hot\" method wasn't executed.
Don't perform redefinition\n");
}
The agent then syncs with the debuggee, waiting for it finish up.
What the test expects is that waitForRedefinitionStarted() in the
debuggee will time out after two seconds while waiting for fire ==
1 (which it thinks will will always happen because it was set to
-1). When it times out, the test does appear to exit properly with,
but with the following in the log, which is intended:
# ERROR: Redefinition not started. Maybe running with -Xcomp. Test
ignored.
However, sometimes before waitForRedefinitionStarted() times out,
the hot method is called enough times to trigger compilation. So
another CompiledMethodLoad event arrives, and this time fire is set
to 1. Because of this, waitForRedefinitionStarted() doesn't time
out and returns with an indication that the redef has started.
After this waitForRedefinitionCompleted() is executed. It waits for
the redef to complete, but it never does since the agent decided
not to do the redef when it saw fire == -1. So
waitForRedefinitionCompleted() times out after 10 seconds and the
test fails, with:
# ERROR: Redefinition not completed.
Actually the above error is not really what causes the failure.
When the above error is detected, no error status is set and the
test continues as if the redef had been done. So then the logic
that detects if the redef was done properly ends up failing, and
that's where the test actually indicates a failure status. You see
a whole bunch of other errors in the log because of all the checks
that fail.
The fix is to not abandon the test when the first
CompiledMethodLoad event is before the hot method was called.
Instead just leave fire==0 and wait for the next CompiledMethodLoad
event that is triggered after the method is called enough times to
be recompiled. I'm not sure why it was not originally done this
way. Possibly the recompilation did not happen reliably, but I have
not run into this problem. The other changes in redefclass030.c are
just cleaning up debug tracing.
Another fix was to properly set the error status when
waitForRedefinitionStarted() or waitForRedefinitionCompleted()
times out, although this is just a safety net and I didn't run into
any cases where this happened after fixing the CompiledMethodLoad
event handling. So in general the changes in redefclass030.java
were not needed, but provide better error handling.
thanks,
Chris