RFR: 8066827: Remove ReferenceProcessor::clean_up_discovered_references()
jon.masamitsu at oracle.com
Wed Dec 17 15:17:24 UTC 2014
Change looks good but I would suggest a different wording in
one of the comments.
+ // 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.
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
> 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.
> 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.
More information about the hotspot-gc-dev