RFR: 8066827: Remove ReferenceProcessor::clean_up_discovered_references()

[Jon Masamitsu]

Kim,

Change looks good but I would suggest a different wording in
one of the comments.

http://cr.openjdk.java.net/~kbarrett/8066827/webrev/src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp.udiff.html

+  // references lists.  Abandon those references, since some
+  // of them may have become unreachable due to later mutator
+  // activity, and the compacting collector we're about to run
+  // won't see them as live.

// reference lists.  Abandon those references since the STW collector will
// redo discovery more precisely (will not be subject to floating garbage).

I think referring to liveness and mutator activity, while correct, makes it
sound more complicated then needed.  I think the overriding point is that
discovery is going to be redone and redone more precisely since it's STW.

I can sponsor when you get all the reviews.

Jon

On 12/11/2014 11:28 PM, Kim Barrett wrote:
> Note: I will need a sponsor for this change.
>
> Please review this change to replace the one call to
> ReferenceProcessor::clean_up_discovered_references() with a call to
> ReferenceProcessor::abandon_partial_discovery(), and then remove the
> clean_up function and some follow-on cleanup.
>
> As noted in the bug report, the benefit from passing a partial
> discovery list from the CMS collector to the compacting collector is
> questionable, while incurring significant complexity and appearing to
> impose a performance cost on the reference processing of other
> collectors, e.g. the need for
> DiscoveredListIterator::update_discovered().
>
> The change to disable and abandon rather than "clean up" the
> discovered list allows us to change the call to
> ReferenceProcessor::enable_discovery() by
> CMSCollector::do_compaction_work() to pass true values for both
> arguments, rather than false values as previously needed.  As a result
> of this, the first argument (verify_disabled) is no longer needed, as
> all callers always pass true.  Also as a result, the only caller
> providing a false value for the second argument (verify_no_refs) is
> the destructor for NoRefDiscovery.  So I've eliminated the first
> argument and made the second default to true, and updated all of the
> callers accordingly (all but ~NoRefDiscovery passing no arguments).
>
> The bug report suggests that the calls to
> DiscoveredListIterator::update_discovered() are no longer needed and
> that function can also be removed.  I'm deferring that (I'll be filing
> a new RFE) because that's a more complex change to analyze and test,
> so I'm separating it from this much simpler to understand change.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8066827
>
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8066827/webrev/
>
> Testing: JPRT, local jtreg of gc tests
> refworkload with CMS as the collector, with TraceCMSState enabled so I
> could examine the logs and verify some of the tests passed through the
> changed code; there were 113 occurrences with an iteration count of 1.
>
> JPRT was after Bengt's recent changes to include gc jtreg tests.
>