RFR: 8072498: Multi-thread JNI weak reference processing

[Thomas Schatzl]

Hi Kim,

On Tue, 2018-07-31 at 15:23 -0400, Kim Barrett wrote:
> Please review this change to WeakProcessor to support processing by
> multiple threads in parallel.  This change uses the WorkGang
> infrastructure to provide tasking and thread management.  The number
> of threads to use may be determined ergonomically, based on
> ReferencesPerThread.
> 
> At this time, only G1 makes use of this change.  CMS is deprecated,
> so we're not spending effort on enhancements of it.  ParallelGC
> doesn't use the WorkGang mechanism for parallelism, leading to the
> same issues here as led to not making ParallelGC's j.l.r.Reference
> processing use an ergonomically determined number of threads. We
> should fix JDK-8204951 before trying to make ParallelGC use the
> parallel form of WeakProcessor.
> 
> As part of this, introduced WeakProcessorPhases (to enumerate and
> manipulate the phases) and WeakProcessorPhaseTimes (for collecting
> and reporting timing information for the phases.
> 
> 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

  - g1GCPhaseTimes.hpp:130: not sure if the removal of the previously
unused _cur_ref_enq_time_ms member should be done here, but I don't
really mind to remove it here. It would be a very trivial change where
the overhead for doing so might be too high.

  - weakProcessor.cpp:73: I am totally okay with the

size_t n_workers = 1 + (ref_count / ReferencesPerThread);

formula, but maybe "ref_count + ReferencesPerThread - 1) /
ReferencesPerThread" which seems more common would be as fine.
Note that you may ignore this comment :)

  - in WeakProcessor::weak_oops_do() there is this optimization that if
nworkers (which other GC code code always writes out as "num_workers"
or "n_workers" :)) is one, it calls the method explicitly.

I would prefer if this optimization would be performed more globally in
e.g. WorkGang::run_task(), or in a static method somewhere that is
called.

We removed all that special-casing in the JDK 9 time frame (https://bug
s.openjdk.java.net/browse/JDK-6979279 (for G1)) with the intent to do
this in a more generic fashion as described above at some point; we are
aware that this is slower. We just did not get to changing it yet. I
still would kind of prefer that we do not start doing that again, but
then again one could remove that at that point too.

  - weakProcessor.inline.hpp, although a new file, has copyright year
2017.

  - we should at least in the gc team at some point come to a rule on
how to format static consts :) Looking at the GC code all possible
"reasonable" styles are used.

Otherwise looks good.

Thanks,
  Thomas