8189360: JvmtiExport::weak_oops_do is called for all JNIHandleBlock instances

[Stefan Karlsson]

Hi Per,

On 2017-10-17 23:43, Per Liden wrote:
> Hi,
> 
> On 2017-10-16 17:40, Stefan Karlsson wrote:
>> Hi all,
>>
>> Please review this patch to move the call of the static 
>> JvmtiExport::weak_oops_do out of the JNIHandleBlock::weak_oops_do 
>> member function into the new WeakProcessor.
>>
>> Today, this isn't causing any bugs because there's only one instance 
>> of JNIHandleBlock, the _weak_global_handles. However, in prototypes 
>> with more than one JNIHandleBlock, this results in multiple calls to 
>> JvmtiExport::weak_oops_do.
>>
>> http://cr.openjdk.java.net/~stefank/8189360/webrev.00/
>> https://bugs.openjdk.java.net/browse/JDK-8189360
> 
>    30 void WeakProcessor::unlink_or_oops_do(BoolObjectClosure* is_alive, 
> OopClosure* keep_alive, VoidClosure* complete) {
>    31   JNIHandles::weak_oops_do(is_alive, keep_alive);
>    32   if (complete != NULL) {
>    33     complete->do_void();
>    34   }
>    35
>    36   JvmtiExport::weak_oops_do(is_alive, keep_alive);
>    37   if (complete != NULL) {
>    38     complete->do_void();
>    39   }
>    40 }
> 
> Should you really be calling complete->do_void() twice here. It seems to 
> me that doing it once, after both calls to weak_oops_do() would mimic 
> what the old code did?

You're right. I've update the code to only call the "complete" closure 
at the end of the function.

FYI, the latest revision of the patch for 8189360 also updated the name 
unlink_or_oops_do to weak_oops_do.

I've also taken the liberty to implement oops_do as a call to 
weak_oops_do. This way we only have to list the calls to the individual 
containers once.

New webrevs:
  http://cr.openjdk.java.net/~stefank/8189360/webrev.01.delta
  http://cr.openjdk.java.net/~stefank/8189360/webrev.01

Thanks,
StefanK

> 
> cheers,
> Per
> 
>>
>> This patch builds upon the patch in:
>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-October/028684.html 
>>
>>
>> Tested with JPRT.
>>
>> Thanks,
>> StefanK