RFR(M/L): 7145569: G1: optimize nmethods scanning
thomas.schatzl at oracle.com
Wed Jun 19 15:57:56 UTC 2013
thanks a lot for considering my comments; inline are answers to your
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:
> > 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.
> > - 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?
More information about the hotspot-gc-dev