RFR 8048268: G1 Code Root Migration performs poorly

[Mikael Gerdin]

On Tuesday 02 September 2014 09.49.19 Jon Masamitsu wrote:
> On 9/1/2014 12:47 AM, Mikael Gerdin wrote:
> > Jon,
> > 
> > On 2014-08-30 01:12, Jon Masamitsu wrote:
> >> Mikael,
> >> 
> >> When an nmethod is made not_entrant,
> >> the  G1CodeRootSet::remove() is called
> >> and if the entries in the table go to 0, the
> >> table is deleted (right?).   And then a new
> >> table is reallocated if needed again (right?)
> >> If yes and yes, why not just  keep the table?
> > 
> > Yes and yes.
> > I was just trying to keep memory usage down, I guess.
> > Also, since there is no current policy for going back to a smaller
> > table if the number of entries goes down this is a way to shrink the
> > size if the amount of code roots pointing into a region changes
> > without the region being evacuated.
> 
> OK.  That's sufficient reason.
> 
> Changes look good.  I have not looked at some of the recent renaming
> but don't wait for me on that since Thomas and Bengt has looked at
> that.

Thanks Jon.

I plan to push this tomorrow morning Stockholm time.

/Mikael

> 
> Reviewed.
> 
> Jon
> 
> > I'm not sure if it's worth it to keep the code for deallocating the
> > table if the size goes to 0, though.
> > 
> > /Mikael
> > 
> >> Jon
> >> 
> >> On 8/26/2014 8:42 AM, Mikael Gerdin wrote:
> >>> Hi,
> >>> 
> >>> In order to combat the spikes in code root migration times I suggest
> >>> that we
> >>> reimplement the code cache remembered set using hash tables instead of
> >>> the
> >>> current chunked array variant.
> >>> 
> >>> While we're at it I suggest that we get rid of the entire migration
> >>> phase and
> >>> update the code cache remembered set during the parallel RSet scanning
> >>> phase.
> >>> The contains()-check when adding during RSet scanning is designed to
> >>> be lock-
> >>> free in order to reduce contention on the HRRS locks.
> >>> This led me to remove some contains-checks in asserts since they were
> >>> done
> >>> during a phase where operations which could not guarantee lock-free
> >>> reads to
> >>> succeed were performed.
> >>> 
> >>> Testing: Kitchensink 14hrs, JPRT, Aurora perf testing of several
> >>> industry
> >>> benchmarks and CRM Fuse (where it actually makes a difference since we
> >>> had
> >>> 300ms spikes in code root migration times).
> >>> 
> >>> The table sizes in G1CodeRootSet are completely unscientific but seem
> >>> to work
> >>> good enough for now. An even larger table size could possibly be
> >>> considered
> >>> for pathological cases where we get thousands of nmethods (as can
> >>> occur in CRM
> >>> Fuse) but currently the two sizes seem good enough.
> >>> 
> >>> This change depends on "JDK-8056084: Refactor Hashtable to allow
> >>> implementations without rehashing support" since the remembered sets
> >>> are
> >>> allocated and deallocated I needed to allow for deallocation of
> >>> instances of
> >>> HashtableEntry and deallocation of freelist contents.
> >>> 
> >>> Webrev:
> >>> http://cr.openjdk.java.net/~mgerdin/8048268/nm-hashtable/webrev/
> >>> Buglink: https://bugs.openjdk.java.net/browse/JDK-8048268
> >>> 
> >>> a note about g1RemSetSummary.cpp
> >>> the code failed to update _max_code_root_mem_sz, so the code to list
> >>> the most
> >>> expensive code root remset was broken.
> >>> 
> >>> /Mikael