RFR(M/L): 7145569: G1: optimize nmethods scanning

[Thomas Schatzl]

Hi John,

  thanks a lot for considering my comments; inline are answers to your
questions.

On Tue, 2013-06-18 at 09:28 -0700, John Cuthbertson wrote:
> Hi Thomas,
> 
> Thanks for looking at the changes. Replies inline....
> 
> On 6/14/2013 3:15 AM, Thomas Schatzl wrote:
> > Hi John,
> >
> >    I had some initial look at the code, first comments (mostly about
> > typos, comments) below:
> >
> > On Thu, 2013-06-13 at 15:30 -0700, John Cuthbertson wrote:
> >> Hi Everyone,
> >>
> >> Can I have a few volunteers look over the changes for this CR - ideally
> >> I would like at least one person from the compiler team and one from the
> >> GC team.
> > - the patch does not seem to apply to latest hotspot-gc repository here,
> > getting a few failed hunks.
> 
> The webrev.all/g1-nmethod-scanning.patch applies just fine for me:
> 

Not sure.

> > nmethod.cpp:
> >
> > - Universe::heap()->register_nmethod(this); seems to be always paired
> > with CodeCache::add_scavenge_root_nmethod(this). Maybe it is useful to
> > put the nmethod registration in that method.
> 
> I would rather not have "registration" dependent upon not being on the 
> scavengable nmethod list. I can see in the future that registration may 
> actually perform adding to the scavengable list but I would like to keep 
> them separate for now.
> 

Okay.

> > - worker_i should be an uint; there has been a pre-hs24 (7121618) issue
> > that changed worker_i variables across all collectors from int to uint.
> > Recently the use of "int" has crept in into g1 code again. If it causes
> > trouble (because int is used in g1), leave it - there is a new CR
> > (8016302) to fix that at once.
> 
> I would prefer to leave this to the other CR. The changes really started 
> to ripple through the rest of the code.

Yes, I guessed so.

> > - In HeapRegion::hr_clear(), it may be interesting to not only clear the
> > code root list, but deallocate it. Depending on the size of this list
> > (do you have numbers?) this may lead to excessive memory use of free
> > regions' HeapRegions. I.e. there have been too many issues recently
> > about G1 taking too much space by default :) In the same direction,
> > maybe the default size of 10 could be lowered. Eg. with 60k+ region
> > heaps, this will add 4M+ to the heap at startup.
> > Then again, this could be done in another cr when fixing the general
> > issue.
> 
> I'm OK with this. The down side is that 
> GrowableArray::clear_and_deallocate() is private so I have to delete the 
> pointer and allocate anew. Is this OK?

This is fine with me.

> > - in remove_strong_code_root() the code tries to remove multiple entries
> > of a given nmethod. How is it possible to get multiple duplicate
> > entries, assuming that push_strong_code_root() already does not push an
> > nmethod if it is already in the list?
> 
> Good point. I don't think it should. Previously I was only checking the 
> top most element in the list for duplication. Is it better to defend 
> against it or just guarantee it?

I am not sure what you mean here in terms of code... :) Without looking
again when is called what, I would simply try to avoid adding duplicates
into the lists, and maybe in debug mode check that during removal.

Could you provide an updated webrev?

Thanks,
  Thomas