RFR 8170870: Fix synchronization of access to PackageEntryTables and ModuleEntryTables

[serguei.spitsyn at oracle.com]

Hi Harold and David,


On 12/21/16 15:48, David Holmes wrote:
> Hi Harold,
>
> On 22/12/2016 12:00 AM, harold seigel wrote:
>> Hi,
>>
>> Please review this fix for bug JDK-8170870.  The fix adds a missing
>> load_ptr_acquire and fixes some asserts.
>
> Sorry but I do not understand these changes. The existing asserts show 
> that the code is executed at a safepoint or with the Module_lock held 
> so how does this suddenly become lock-free? If you are "simply" 
> intending this code to be allowed to be called other than at a 
> safepoint or with the Module_lock held, then it may require a lot more 
> than a load_ptr_acquire to make it safe!

I'm puzzled too.


>
> The bug synopsis and description do not describe the problem being 
> addressed - or even explain what the problem is, or the proposed solution.

I guess, the suggestion was from Markus in one of the comments.

As I understand, the JFR needs to iterate over ModuleEntry* and 
PackageEntry* objects.
It does it in a couple of different contexts: during GC/safepoints and 
at special collection points outside safepoints.

For example, it makes calls to the functions:
   - ModuleEntry::has_reads()
   - ModuleEntry::module_reads_do(module_reads_do );
   - ModuleEntryTable::~ModuleEntryTable();              <=== Not sure 
if this one is used in JFR
   - ClassLoaderData::modules_do(void f(ModuleEntry*)));
   - maybe something else ...

There are some other cases described by Markus that are related to the 
classes, not modules.

It does not look that now there are any problems with the JFR points 
during GC/safepoints.
I'd guess, it should work well as it is now.

But it is hard to understand the contexts of the special collection points.
These calls need to be also somehow protected/synchronized.
(I would not be surprised if there are some performance requirements here.)
Simply grabbing the Module_lock here looks risky (can be deadlocks prone),
and as such, needs more analysis.

It is why Markus suggested a lock-free kind of synchronization.
But this approach needs to be described with more details and also is
going to impact more code paths including JFR code at GC/safepoints.
(: it seems, the "hazard pointers" from Erik O. are missed here. :)

>
>> It also removes the
>> exported_pending_delete array, improving the management of package
>> qualified export lists.
>
> That seems to be a completely unrelated issue which should have its 
> own bug IMHO.

I agree, separating this into a different bug would simplify things.


Thanks,
Serguei


>
> Thanks,
> David
>
>> Open Webrev:
>> http://cr.openjdk.java.net/~hseigel/bug_8170870/webrev/index.html
>>
>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8170870
>>
>> The fix was tested with the JCK Lang and vm tests, the JTreg hotspot,
>> java/io, java/lang, java/util and other JTReg tests, and the NSK
>> co-located and non-colocated tests.
>>
>> Thanks, Harold
>>