jdk15:
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.jdk15.2/ (full)
src/hotspot/share/prims/jvmtiEnvBase.cpp
old L1029: ResourceMark rm;
It's not clear (to me anyway) why this ResourceMark is removed.
Update: I saw the discussion of "ResourceMark rm" in JDK15 versus
"ResourceMark rm(current_thread)" in JDK16, but that doesn't tell
me why it was necessary to remove that ResourceMark.
src/hotspot/share/prims/stackwalk.cpp
L291: ResourceMark rm;
L292: HandleMark hm;
Since there's a TRAPS parameter, these should be 'rm(THREAD)' and
'hm(THREAD)'.
src/hotspot/share/runtime/biasedLocking.cpp
No comments.
src/hotspot/share/runtime/deoptimization.cpp
No comments.
src/hotspot/share/runtime/vframe.cpp
L461: _lock = lock;
nit - extra space before '='.
src/hotspot/share/runtime/vframe.hpp
L32: #include "runtime/handles.inline.hpp"
nit - new include is out of order; should be after frame.hpp.
src/hotspot/share/runtime/vframeArray.cpp
No comments.
src/hotspot/share/runtime/vframe_hp.cpp
Skipped - no changes.
src/hotspot/share/services/threadService.cpp
No comments.
jdk16:
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.2/ (full)
Same comments as for the JDK15 version. I also compared the two patches
using jfilemerge.
Most of what I have above are nits so I'm good with both of these
patches. I also looked for lifetime issues with the new ResourceMarks
and HandleMarks, but I don't see any issues.
Dan
On 7/22/20 4:14 AM, Thomas Schatzl wrote:
Hi Serguei,
thanks for your review.
On 22.07.20 04:13, serguei.spit...@oracle.com wrote:
Hi Thomas,
The fix looks okay to me.
The 15 fix is identical to 16.
no :) It is very subtle. As mentioned, in jvmtiEnvBase.cpp in the
original code, in jdk15 we had:
line 1029: ResourceMark rm;
in jdk16:
line 1008: ResourceMark rm(current_thread);
Both lines were ultimately removed with this recent change, but still
cause merge errors if you port the patch from one version to the other.
Sorry if that has been confusing.
This file finally was not changed:
src/hotspot/share/runtime/vframe_hp.cpp .
This is an artifact of webrev in conjunction with mq because the
original change had some modifications which were retracted in the -1
webrev there. There will not be any change in any push for that file.
Several files need a copyright comment update.
Provided new webrevs with only comment updates below. Did not do new
testing just for these comment updates though.
What tests do you run?
We need at least tier3 to make sure there are no regressions in the
JVMTI and JDI tests.
The jdk15 .1 patch has been tested with 1.2k of that LocalsAndOperands
test with the options that originally reproduced it in 1/100 cases.
hs-tier1-5, no failures at all.
jdk16 has had testing with the .0 patch doing 1.8k of
LocalsAndOperands.java, tier1-5, and tier8 with JDK-8249676 reinstated
that earlier caused lots of issues there (and none without).
Since there has been no substantial change in how the patch works,
only some refactoring, so I think these are still valid.
See the internal comments in the CR for links.
New webrevs:
jdk15:
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.jdk15.2/ (full)
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.jdk15.1_to_2/ (diff)
jdk16:
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.2/ (full)
http://cr.openjdk.java.net/~tschatzl/8249192/webrev.1_to_2/ (diff)
Thanks,
Thomas
