RFR (M): 8087198: G1 card refinement: batching, sorting
thomas.schatzl at oracle.com
Tue Nov 12 19:27:37 UTC 2019
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
>> RFE: https://bugs.openjdk.java.net/browse/JDK-8087198
>> Webrev: https://cr.openjdk.java.net/~manc/8087198/webrev.00/
>> Old review thread is:
>> 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
>> - 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
> Some initial thoughts, not a full review yet.
> 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.
> 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,
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?
More information about the hotspot-gc-dev