RFR (7xS): 8175554: Improve G1UpdateRSOrPushRefClosure

Tue May 30 22:15:05 UTC 2017

Hi Thomas,

On 05/05/2017 05:13 AM, Thomas Schatzl wrote:
> Hi all,
>
>    recent reviews have made changes necessary to parts of the changeset
> chain.
>
> Here is a list of links to updated webrevs. Since they have apparently
> not been reviewed yet, I simply overwrote the old webrevs.
>
> JDK-8177044: Remove _scan_top from HeapRegion
> http://cr.openjdk.java.net/~tschatzl/8177044/webrev/
>
> JDK-8178148: Log more detailed information about scan rs phase
> http://cr.openjdk.java.net/~tschatzl/8178148/webrev/
>
> JDK-8175554: Improve G1UpdateRSOrPushRefClosure
> http://cr.openjdk.java.net/~tschatzl/8175554/webrev/
Looks good to me.
I only have minor nits.

------------------------------------------------------
src/share/vm/gc/g1/g1OopClosures.hpp
   78   virtual void do_oop(oop* p) { do_oop_nv(p); }
Misaligned with above line.

------------------------------------------------------
src/share/vm/gc/g1/g1RemSet.hpp
  204                   G1UpdateOrScanRSClosure* push_heap_cl,
Rename to reflect new closure name?

------------------------------------------------------
src/share/vm/gc/g1/g1RootProcessor.hpp
Copyright update.

------------------------------------------------------
src/share/vm/gc/g1/g1_specialized_oop_closures.hpp
   45       f(G1UpdateOrScanRSClosure,_nv)         \
Misaligned '\'.

Thanks,
Sangheon

>
> JDK-8178151: Clean up G1RemSet files
> http://cr.openjdk.java.net/~tschatzl/8178151/webrev/
>
> Please consider changing the subject of the email if you are reviewing
> one specific webrev, as this seems helpful to quickly find the context
> but keep the reviews together in the email thread.
>
> Thanks,
>    Thomas
>
> On Tue, 2017-04-11 at 13:30 +0200, Thomas Schatzl wrote:
>> Hi all,
>>
>>    I would like you to ask for reviews of a set of related changes
>> that optimize the way G1 updates/scans/refines cards of the
>> remembered set.
>>
>> As these CRs are highly dependent on each other I would like to have
>> them reviewed and pushed together. Another reason is that all of them
>> are self-contained, which means that they take one or the other
>> detour I want to avoid questions about.
>>
>> The basic premise is that refinement and Scan RS (and Update RS as it
>> is mostly the same) phases are quite slow with G1. There are a lot of
>> low-hanging fruits to be picked here, ranging from micro-
>> optimizations, fixing "wrong" code sharing, re-using (slow) CMS code,
>> doing stuff in
>> non-straightforward ways etc.
>>
>> These changes try to make it better (tm) ;)
>>
>> Results are good, around 10%+ faster scan rs time/card, 10%+ faster
>> update rs/card, refinement also faster (although it is too hard to
>> measure), 8 LOC less (there is more to delete, but that one needs
>> more care. Also these changes add some logging :))
>>
>> Here we go:
>>
>> 8071280: Specialize HeapRegion::oops_on_card_seq_iterate_careful()
>> for
>> use during concurrent refinement and updating the rset
>>
>> CR: https://bugs.openjdk.java.net/browse/JDK-8071280
>> Webrev: http://cr.openjdk.java.net/~tschatzl/8071280/webrev
>>
>> As the title indicates, this change allows specializations of
>> HeapRegion::oops_on_card_seq_iterate_careful() according to current
>> GC
>> phase (concurrent or during gc) to allow to remove some checks.
>> This is mostly a preparatory change for the next ones.
>>
>> 8162928: Micro-optimizations in scanning the remembered sets
>>
>> CR: https://bugs.openjdk.java.net/browse/JDK-8162928
>> Webrev: http://cr.openjdk.java.net/~tschatzl/8162928/webrev/
>>
>> This change removes the use of HeapRegionDCTOC during scanning
>> remembered sets, and uses the faster
>> HeapRegion::oops_on_card_seq_iterate_careful() (measured).
>> HeapRegionDCTOC also depends CMS code which is just bad.
>>
>> 8177707: Specialize G1RemSet::refine_card for concurrent/during
>> safepoint refinement
>>
>> CR: https://bugs.openjdk.java.net/browse/JDK-8177707
>> Webrev: http://cr.openjdk.java.net/~tschatzl/8177707/webrev/
>>
>> This change temporarily duplicates the code in G1RemSet::refine_card
>> for during refinement/gc to allow (initial) specialization of these
>> methods for their purpose.
>>
>> 8177044: Remove _scan_top from HeapRegion
>>
>> CR: https://bugs.openjdk.java.net/browse/JDK-8177044
>> Webrev: http://cr.openjdk.java.net/~tschatzl/8177044/
>>
>> Removes _scan_top and all its awkward handling by explicitly creating
>> a
>> snapshot of the top values of the heap regions. This allows
>> significant
>> specialization of the refine_during_gc() method by subsuming all the
>> various checks at the beginning into a single one.
>>
>> This one is responsible for a large part of the improvement of the
>> update rs phase.
>>
>> 8178148: Log more detailed information about scan rs phase
>>
>> CR: https://bugs.openjdk.java.net/browse/JDK-8178148
>> Webrev: http://cr.openjdk.java.net/~tschatzl/8178148/webrev/
>>
>> Adds some more statistics gathering and logging about what the scan
>> rs
>> phase is actually doing, allowing better performance analysis. This
>> information will also be used further in the future.
>>
>> 8175554: Improve G1UpdateRSOrPushRefClosure
>>
>> CR: https://bugs.openjdk.java.net/browse/JDK-8175554
>> Webrev: http://cr.openjdk.java.net/~tschatzl/8175554/webrev/
>>
>> (This mostly deletes code, so it's still "S" ;))
>>
>> Finally remove the G1ParPushHeapRSClosure and merge it with
>> G1UpdateOrScanRSClosure. You may now notice that
>> G1UpdateOrScanRSClosure is pretty similar to G1ParScanClosure - that
>> is
>> another issue.
>> Also make sure that G1UpdateOrScanRSClosure properly does the
>> humongous
>> and "ext" region handling (which is benign so far).
>>
>> 8178151: Clean up G1RemSet files
>>
>> CR: https://bugs.openjdk.java.net/browse/JDK-8178151
>> Webrev: http://cr.openjdk.java.net/~tschatzl/8178151/webrev/
>>
>> Clean up after all these changes a little in the G1Remset* files,
>> removing obsolete stuff, some renamings, etc.
>>
>> Thanks,
>>    Thomas
>>