RFR (M): 8087198: G1 card refinement: batching, sorting

[Thomas Schatzl]

Hi,

On 12.11.19 18:26, Kim Barrett wrote:
>> On Nov 11, 2019, at 9:26 PM, Man Cao <manc at google.com> wrote:
>>
>> Hi all,
>>
>> Can I have reviews for an updated implementation for batching card
>> refinement?
>> RFE: https://bugs.openjdk.java.net/browse/JDK-8087198
>> Webrev: https://cr.openjdk.java.net/~manc/8087198/webrev.00/
>>
>> Old review thread is:
>> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2015-June/013798.html.
>> Major differences from the 2015 webrev:
>> - New version does not save the MemRegions for the cards in a buffer. I
>> noticed considerable memory overhead with BigRamTester if we save the
>> MemRegions.
>> - New version handles SuspendibleThreadSetJoiner::should_yield() in a more
>> timely fashion. Instead of forcing refining all buffered cards, the new
>> version can abandon the buffered cards.
>> - New version only batches and sorts the cards, not joining and
>> prefetching. I have not investigated whether joining and prefetching help
>> much. I think it is OK to investigate them in a separate RFE later.
>> Please refer to the RFE page for some performance results.
>>
>> For correctness, tested with:
>> - Submit repo: tier1
>> - Local fastdebug build: tier2
>> - Fastdebug stress testing DaCapo h2 and BigRamTester with following
>> option combinations in addition to -XX:+VerifyRememberedSets:
>>    default options
>>   -XX:-G1UseAdaptiveConcRefinement -XX:G1UpdateBufferSize=4
>> -XX:G1ConcRefinementGreenZone=0 -XX:G1ConcRefinementYellowZone=1
>>   -XX:G1ConcRefinementThreads=0
>>
>> -Man
> 
> Some initial thoughts, not a full review yet.

Same here.

> 
> The approach looks good.  It might also simplify some ideas I've been
> playing with in the background.
> 
> I think the decision to defer investigation of additional batching,
> joining, and prefetching is fine.

I already split this CR into two.

> 
> I'd like to see more performance testing.  I'll probably do some, once
> I think the change looks more settled.

 From a functional POV hs-tier1-5 pass with the change.

> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1ConcurrentRefineThread.cpp
>   114       ResourceMark rm;
> 
> I don't think this is the right place for the ResourceMark.  I think
> it belongs in G1RefineBufferedCards.  Wrapping a ResourceMark around
> an unbounded iteration isn't really safe; if the allocations and frees
> are all nested properly it will work, but if not it can explode.
> 
> All this assumes the ResourceMark to deal with the temporary buffer
> allocated by G1RefineBufferedCards is still needed.  If there are no
> temporary buffers...
> 

I also noticed those, there are two places where ResourceMarks without 
any obvious use are missing, probably leftovers of debugging code?

--------------------------------

The code snippet:

  365   G1RefineBufferedCards buffered_cards(node,
  366                                        buffer_size(),
  367                                        counter);
  368   bool result = buffered_cards.refine(worker_id);

could probably be wrapped into a method as it is done twice. The 
initialization of the G1RefineBufferedCards breaks a bit the flow of the 
code without later use of the buffered_cards variable.

Maybe it is used in follow-up patches in these places?

Thanks,
   Thomas