RFR: 8072498: Multi-thread JNI weak reference processing

[Roman Kennke]

Am 09.08.2018 um 20:57 schrieb Kim Barrett:
>> On Aug 8, 2018, at 10:27 AM, Kim Barrett <kim.barrett at oracle.com> wrote:
>>
>>> On Aug 8, 2018, at 10:11 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
>>>
>>> Hi Kim,
>>>
>>> On Tue, 2018-07-31 at 15:23 -0400, Kim Barrett wrote:
>>>> […]
>>>> CR:
>>>> https://bugs.openjdk.java.net/browse/JDK-8072498
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~kbarrett/8072498/open.00/
>>>>
>>>> Testing:
>>>> mach5 tier1-3, hs-tier4-5.
>>>>
>>>> Local and mach5 testing of TestGCBasherWithG1 and TestSystemGCWithG1,
>>>> with the tests modified to use a smaller non-default value of
>>>> -XX:ReferencesPerThread, to examine the logging output and verify
>>>> multi-threaded execution.
>>>>
>>>
>>> - g1ConcurrentMark.cpp:1673: I assume that using the serial version
>>> of weak processing has been used here because of logging concerns. Is
>>> there an upcoming change that fixes that?
>>> One could create a local instance of a WeakProcessorPhaseTimes and just
>>> take the interesting total time taken from it and print it. Or just
>>> print it the same way as for the young gcs - I understand this is ugly,
>>> but as long as there is no other way...
>>>
>>> - same for g1FullCollector.cpp:217 and g1FullGCAdjustTask:101
>>
>> These are using the parallel versions.  Maybe overloading the weak_oops_do
>> name with one that takes a workgang is too subtle?
> 
> To be clear, these are calling this overload:
> 
>   template<typename IsAlive, typename KeepAlive>
>   weak_oops_do(WorkGang*, IsAlive*, KeepAlive*, uint log_indent)
> 
> which stack allocates a temporary WeakProcessorPhaseTimes, calls the
> parallel overload taking a phasetime as the fourth argument, and then
> logs the phasetime with the requested indentation.
> 
> New webrevs without the external optimization of run_task:
> full: http://cr.openjdk.java.net/~kbarrett/8072498/open.01/
> incr: http://cr.openjdk.java.net/~kbarrett/8072498/open.01.inc/
> 
> And still need another reviewer.



Wouldn't it be much more useful to turn the design around such that
weak_oops_do() can be called from a worker, like e.g.
Threads::possibly_parallel_oops_do(), and leave all the actual wiring up
of workers to the GC? That would allow to be used in GC that doesn't
even use WorkGang (e.g. CMS) and probably also helps with ParallelGC? I
know it would fit better into the picture in Shenandoah, and seems more
consistent with other parallel oops processing routines. Not sure if it
would be possible at all, though.

Side-note and somewhat off-topic: the specialization via templates is
probably useful for performance, especially for cheap stuff like
BoolObjectClosures. I was wondering a while back if that would be
something to apply to the rest of ReferenceProcessor, and, infact, to
the rest of root scanning routines too. ? I know it's probably a lot of
work to get that right...

I will take a closer look at all this tomorrow. It's far too late here
and I'd probably get it all wrong ;-)

Cheers,
Roman