RFR (S/M): 8202017: Merge Reference Enqueuing phase with phase 3 of Reference processing

Kim Barrett kim.barrett at oracle.com
Thu Apr 26 23:03:47 UTC 2018


> On Apr 24, 2018, at 5:26 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> Hi Kim,
> 
> On Mon, 2018-04-23 at 19:10 -0400, Kim Barrett wrote:
>>> On Apr 19, 2018, at 3:14 PM, Thomas Schatzl <thomas.schatzl at oracle.
>>> com> wrote:
>>> 
>>> Hi all,
>>> 
>>> can I have reviews for this change that merges the Reference
>>> Enqueuing phase of reference processing with the respective phase 3
>>> of actual Reference Processing?
>>> 
> [...]
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8202017
>>> Webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8202017/webrev/
>>> Testing:
>>> hs-tier1-4
>>> 
>>> Thanks,
>>> Thomas
>> 
>> Looks good.  Just a couple very minor things.
>> 
>> -------------------------------------------------------------------
>> ----------- 
>> src/hotspot/share/gc/shared/referenceProcessor.cpp
>> 607     _refs_lists[i].set_head(NULL);
>> 608     _refs_lists[i].set_length(0);
>> 
>> 790        refs_lists[i].set_head(NULL);
>> 791        refs_lists[i].set_length(0);
>> 
>> Maybe move these into process_phase3?
> 
> Done.
> 
>> 
>> -------------------------------------------------------------------
>> ----------- 
>> src/hotspot/share/gc/g1/g1CollectedHeap.hpp
>> 515   // If during an initial mark pause we install a pending list
>> head which is not otherwise reachable
>> 
>> Perhaps s/install/may install/
>> 
> 
> Done.
> 
>> src/hotspot/share/gc/shared/genCollectedHeap.cpp
>> 518       rp->disable_discovery();
>> 
>> Ick!  This whole little dance around discovery_is_atomic,
>> enqueueing_is_done, &etc looks like it is related to CMS and the
>> distinction between CMS and Serial.  See also the TODO-ish block
>> comment at line 489.
>> 
>> This looks ripe for refactoring as part of the cleanup of CMS-
>> specific intrusions.  Probably should have a followup RFE for this.
> 
> I will file an RFE for this.
> 
> New webrevs:
> http://cr.openjdk.java.net/~tschatzl/8202017/webrev.0_to_1 (diff)
> http://cr.openjdk.java.net/~tschatzl/8202017/webrev.1/ (full)
> 
> Thomas

In the comment for G1CollectedHeap::make_pending_list_reachable():
 516   // otherwise reachable ensure that it is marked in the bitmap for concurrent marking

s/reachable ensure/reachable. Ensure/

Otherwise looks good.  No need for a new webrev for that comment change.




More information about the hotspot-gc-dev mailing list