RFR [1/7]: 8197569: Refactor eager reclaim for concurrent remembered set rebuilding (original) (raw)

sangheon.kim sangheon.kim at oracle.com
Tue Mar 6 23:28:46 UTC 2018

Hi Thomas,

Thank you for these bunch of patches for rebuilding RemSet concurrently. This is really nice.

On 03/06/2018 05:17 AM, Thomas Schatzl wrote:

Hi Stefan,

thanks for looking into this. On Mon, 2018-03-05 at 16:11 +0100, Stefan Johansson wrote: Thanks for splitting this change up Thomas,

On 2018-03-05 15:18, Thomas Schatzl wrote: Hi all,

can I have some reviews for the following change that refactors the eager reclaim condition so that it can be later used for the policy that determines for which regions G1 should rebuild the remembered set? (And it's a reasonable refactoring on its own btw). CR: https://bugs.openjdk.java.net/browse/JDK-8197569 Webrev: http://cr.openjdk.java.net/~tschatzl/8197569/webrev/ Look good, just one thing: src/hotspot/share/gc/g1/g1CollectedHeap.cpp 2542 bool humongousregioniscandidate(G1CollectedHeap* heap, HeapRegion* region) const { ... 2594 return obj->istypeArray() && 2595 G1CollectedHeap::heap()- ispotentialeagerreclaimcandidate(region); You could use the passed in heap parameter instead of using G1CollectedHeap::heap(). Done. Also renamed the parameter to "g1h" as used everywhere else. Good but still there are some places using 'g1'. It would be good idea to fix, of course in other CR.

Another, maybe even nicer, solution would be to move ispotentialeagerreclaimcandidate() to HeapRegion, and just call region->ispotentialeagerreclaimcandidate(). I did not do this for the following reasons: - ispotentialeagerreclaimcandidate may depend on global state in the future. E.g. when trying to eagerly reclaim objArrays or similar the result depends on whether we are currently marking or not. I do not like making HeapRegion dependent on e.g. G1CollectorState :) (any more than it already may be). - (almost) all eager reclaim related code has until now been in G1CollectedHeap. Such a change would go against this, and run kind of contrary to my intent to move away from using HeapRegion as dumping ground for anything loosely related to it out of convenience, but only as a representation for that memory. Either way is kind of fine for me right now, so let's see what others say? Sounds reasonable to me.

New webrevs: http://cr.openjdk.java.net/~tschatzl/8197569/webrev.0to1/ (diff) http://cr.openjdk.java.net/~tschatzl/8197569/webrev.1/ (full) webrev.1 seems good to me.

Thanks, Sangheon

Thanks, Thomas

More information about the hotspot-gc-dev mailing list