RFR(S): 8015237: Parallelize string table scanning during strong root processing

[Thomas Schatzl]

Hi John,

On Tue, 2013-05-28 at 15:31 -0700, John Cuthbertson wrote:
> Hi Thomas,
> 
> Thanks for taking a look. Replies inline....
> 
> On 5/27/2013 1:04 AM, Thomas Schatzl wrote:
> > Hi John,
> >
> > On Fri, 2013-05-24 at 15:36 -0700, John Cuthbertson wrote:
> >> Hi Everyone,
> >>
> >> On 5/24/2013 3:19 PM, John Cuthbertson wrote:
> >>
> >>> Hi Everyone,
> >>>
> >>> Can I have a couple of reviewers look over these changes - the
> >>> webrev is: http://cr.openjdk.java.net/~johnc/8015237/webrev.0/
> >>>
> >>> Summary:
> >>> On some workloads we are seeing that the scan of the intern string
> >>> table (among others) can sometimes take quite a while. This showed
> >>> up on some FMW workloads with G1 where the scan of the string table
> >>> dominated the pause time for some pauses. G1 was particularly
> >>> affected since it doesn't do class unloading (and hence pruning of
> >>> the string table) except at full GCs. The solution was to change the
> >>> string table from being considered a single root task and treat
> >>> similarly to the Java thread stacks: each GC worker claims a given
> >>> number of buckets and scans the entries in those buckets.
> >>>
> >>> Testing
> >>> Kitchensink; jprt; GC test suite. With all collectors.
> > Looks good. A few minor nits:
> >
> > So maybe this could be factored out into a helper routine (e.g.
> >
> > StringTable::process_entries(OopClosure * f, uint start_idx, uint
> > end_idx) {
>>
>> [...]
>>
> >      }
> >    }
> > }
> >
> > and called by both StringTable::possibly_parallel_oops_do()
> > and ::oops_do() with appropriate parameters?
> 
> That's a good idea. A better name might be buckets_do, process_buckets, 
> oops_buckets_do (as in oops_interpreted_frame_do).

oops_range_do()/oops_do_[bucket_]range()? I am not really picky about
the name.

> > (maybe find a better name for that method :)
> >
> > - in SharedHeap.cpp:163 there is a misplaced colon in the call to
> > Threads::possibly_parallel_oops_do(); this has been the case before as
> > well, so it's not really an issue.
> 
> OK. Sure I can clean that up.
> 
> > - this is just a suggestion: if this is a problem (it does not seem so),
> > and of course it also depends on the size of the table (the number of
> > synchronizations), why not make the size of the chunks a function of the
> > number of threads and the size of the string table?
> 
> I thought about this but decided to leave it for another CR. I actually 
> like Per's idea of basing the number of buckets on the value of 
> CACHE_LINE_SIZE so will probably go with that.
> 
> Expect a new webrev soon.

Thanks a lot,
  Thomas