RFR [M][7/7]: 8197932: Better split work in rebuild remembered sets phase (original) (raw)
sangheon.kim sangheon.kim at oracle.com
Wed Mar 28 17:03:21 UTC 2018
- Previous message (by thread): RFR [M][7/7]: 8197932: Better split work in rebuild remembered sets phase
- Next message (by thread): RFR [0/7]: Rebuild remembered sets concurrently
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Hi Thomas,
On 03/28/2018 04:23 AM, Thomas Schatzl wrote:
Hi,
On Tue, 2018-03-27 at 10:50 -0700, sangheon.kim wrote: Hi Thomas,
Finally the last piece! :) yes :) Unfortunately there are already a few more changes out for review, although much smaller in scope and complexity. :)
[...]
New webrev: http://cr.openjdk.java.net/~tschatzl/8197932/webrev.2to3 (diff) http://cr.openjdk.java.net/~tschatzl/8197932/webrev.3 (full) webrev.3 looks good! Minor nits below.
============================= src/hotspot/share/gc/g1/g1RemSet.cpp 805 void movetonext() { 810 oop next() const { (Ignore if you don't agree) - I like the idea of using iterator here. But for me, current naming seems misleading. Current movetonext() seems next(). And something else for current next(). tooop()? I would like to keep it as is until we in the team figured out how to do iterators in hotspot, and then change as needed. Apart from the naming this style is 1:1 compatible to STL iterators (i.e. movetonext() -> "++" operator, next() -> * operator) (which although I expect us to use them in the future, I have no particular preference for any style), so I chose that. Okay!
============================= src/hotspot/share/gc/g1/g1RemSetTrackingPolicy.cpp 37 // All non-young and non-closed archive regions need to be scanned for references; 38 // At every gc we gather references to other regions in young, and closed archive 39 // regions by definition do not have references going outside the closed archive. - Add some explanation about r->isfree() case as well? I will fix this before pushing. OK
============================= src/hotspot/share/gc/g1/g1globals.hpp 261 range(4 * K, 32 * _M) _ - Just question. Why the min is 4K? 4k is like 1024 references max. This seemed to be a small enough increment to not cause too much safepointing delay for the smallest machine we are going to run with G1. Thanks for the answer, agree.
I am going to push this change as is - your review did not seem to pose any significant issue. Otherwise there is enough opportunity to fix this immediately. Thank you for the nice patches!!
Sangheon
Thanks, Thomas
- Previous message (by thread): RFR [M][7/7]: 8197932: Better split work in rebuild remembered sets phase
- Next message (by thread): RFR [0/7]: Rebuild remembered sets concurrently
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]