RFR (S): 8212911: Unify and micro-optimize handling of non-in-collection set references in oop closures

Kim Barrett kim.barrett at oracle.com
Tue Oct 30 00:54:48 UTC 2018


> On Oct 25, 2018, at 5:45 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> Hi all,
> 
> On Wed, 2018-10-24 at 13:36 +0200, Thomas Schatzl wrote:
>> Hi all,
>> 
>>  can I have reviews for this small change that does some minor
>> cleanup and micro-optimization of our main g1 stw/concurrent gc
>> closures?
>> 
>> [...]
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8212911
>> Webrev:
>> http://cr.openjdk.java.net/~tschatzl/8212911/webrev/
>> Testing:
>> hs-tier1-3, several hs-tier1-5 runs with other changes, benchmark
>> suite etc.
> 
>  Stefan pointed out that _from_in_young is only used in
> G1ScanEvacuatedObjClosure any more, so it could be moved from
> G1ScanClosureBase.
> 
> Also we brainstormed a bit about the name of _from_in_young, and agreed
> on changing this to _scanning_in_young.
> 
> New webrevs:
> http://cr.openjdk.java.net/~tschatzl/8212911/webrev.1 (full)
> http://cr.openjdk.java.net/~tschatzl/8212911/webrev.0_to_1 (diff)
> 
> Thanks,
>  Thomas

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1OopClosures.inline.hpp
 178   } else {
 179     if (HeapRegion::is_in_same_region(p, obj)) {
 180       return;
 181     }

I think this would be easier to read if it were written as

 178   } else if (!HeapRegion::is_in_same_region(p, obj)) {

There are similar snippets at line 86 and line 200.  I don’t need
a new review if you want to make these changes.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1OopClosures.hpp
  91     G1ScanClosureBase(g1h, par_scan_state), _scanning_in_young(false) { }

I kind of dislike the initialization of _scanning_in_young to false.
But that seems consistent with previous usage of _from.  For catching
bugs, a kind of tri-state and an RAII-style setter (from "unset" to
true or false then back) might be better.  If you like that idea but
don't want to do it right now, feel free to file an RFE.  (The reset
back to "unset" could be DEBUG_ONLY, along with all of the relevant
checking being in asserts.)

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

Looks good, up to the above optional to address issues.




More information about the hotspot-gc-dev mailing list