RFR: 8220465: Use shadow regions for faster ParallelGC full GCs

Thomas Schatzl thomas.schatzl at oracle.com
Mon Dec 2 16:45:20 UTC 2019


Hi,

On 30.11.19 12:52, Haoyu Li wrote:
> Hi Stefan,
> 
> Thanks for your reviewing. If there are any further problems in the 
> code, please feel free to contact me! I am always more than happy to 
> improve the code.

Looks good, with the following minor issues:

> > >> - I think there is a missed optimization opportunity in (now)
> > >> PSParallelCompact::initialize_shadow_regions(). There, the code
> > >> initializes the "free" region ids to region_at_top+1 to
> > >> end_region of a particular space.
> > >>
> > >> If the top for a given space is at a region boundary (e.g. if
> > >> a space is empty, which is probably common for one of the
> > >> survivor spaces), you loose a single region per space.
> > >>
> > >> One reason might that the code uses region "0" as sentinel to
> > >> indicate "there is no shadow region available" in
> > >> ParCompactionManager::acquire_shadow_region().
> > >>
> > >> This could be fixed by improving the code in
> > >> PSParallelCompact::initialize_shadow_regions() and use a sentinel
> > >> region value of (size_t)~0 (as an explicit constant).
> > >>
> > >> Even if you do not change this, please introduce an explicit
> > >> onstant for this sentinel value. This makes the code more self-
> > >> explanatory.
> > >>
> > >> Sorry for the misleading +1 operation. The +1 can be safely
> > >> removed. The sentinel value 0 does not cause this design because
> > >> the first region (in old space) cannot be a shadow region.

I was trying to come up for a reason that "the first region cannot be a 
shadow region", could you explain? What about if compaction starts off 
with empty old gen? In this case probably the situation can't occur, but 
still it seems more complicated than an obviously invalid shadow region 
id (which is far beyond the heap).

Please change to use the ~0 sentinel (with a properly named constant) 
instead - this is much easier to reason about than otherwise.

- psParallelCompact.cpp: line 3064: RegionSize is now unused, the line 
can be deleted.

- I'd prefer if the push/pop_shadow_region_mt_safe methods used a 
MonitorLocker and a wait/notify scheme for synchronization. The pop 
method locking the _shadow_region_monitor in the infinite loop body (and 
not doing anything else) seems problematic when there is contention (if).

Thanks,
   Thomas



More information about the hotspot-gc-dev mailing list