[15?] RFR (S): 8249192: MonitorInfo stores raw oops across safepoints

[Daniel D. Daugherty]

> 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.spitsyn at 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