RFR(M/L): 6484982: G1: process references during evacuation pauses
bengt.rutisson at oracle.com
Tue Sep 13 18:15:07 UTC 2011
Hi again John, Ramki and Stefan,
Just a quick comment on this topic:
>>>> - only bracket changes. Don't include in thins change.
>>> I had debugging code in here and so left the 'correctly' formatted
>>> code after removing the debugging statements. Since I've already
>>> been thanked for doing this - I would leave it.
>> Sorry for being persistent but we think this is not a good enough
>> reason to leave it in.
> Bengt, I'm not understanding your objection to fixing the formatting
> to be the style we would really want it to be. Are you saying that
> this fixes the formatting, that such formatting changes should not be
> of this changeset? (If so, why not?)
I am definitely not saying that we should not fix formatting and style
issue. Of course we should. And I am also not suggesting creating CRs
for small style changes.
What I am saying is that in this case there are no changes at all in
thread.cpp. The only change is a style change. For such a case I see no
reason to include it in this changeset. I realize that John have been
having more changes in there and then removed them. But when doing the
review it looks like this style fix was just a random fix in a random
file. If you see what I mean.
So, I think we should save this type of style changes to the next time
when we actually do changes that relate to thread.cpp.
I am not going to insist on this being excluded. I just wanted to state
>>>> - You removed inline from void DiscoveredListIterator::remove(),
>>>> will that impact performance?
>>> Possibly. The include dependencies are such that I've been unable to
>>> move these routines into referenceProcessor.hpp or a
>>> referenceProcessor.inline.hpp as compilation errors occur when I
>>> include javaClasses.hpp and systemDictionary.hpp.
> Not now, but i think we should try and fix this in the future. I don't
> understand (although i did see your compiler errors!) why we should
> not be able to make
> whatever method we want to be inline when we want it to be by moving
> its definition
> into a .inline.hpp and including only that in the .cpp's that need it.
> (This might involve
> moving some definitions out of .hpp's and placing them in new
> .inline.hpp's as needed --
> i understand that this can lead to a cascading contagion that moves
> lots of code out
> of ,hpp's and into possibly new .inline.hpp's, so not worth doing as
> part of this changeset.)
>>>> - What does "ser" stand for in "stw_rp_disc_ser"?
>>> Serial. We've made discovery for the STW reference processor non MT.
>>>> - Would be nice to move these two lines into enable_discovery().
>>>> (May need a parameter to disable the check for NoRefDiscovery to
>>>> work properly.)
>>>> assert(!ref_processor_stw()->discovery_enabled(), "Precondition");
>>> OK. Again I would rather this was a separate change for precisely
>>> the reason you mention.
>> We think it fits well into this change. But if you don't want to do
>> it now, could you please file a CR to make sure that we don't forget
>> to do it?
> I must agree with Bengt that this may be a small local change that may
> be worth
> doing here. But I also sympathize with you for avoiding more changes
> that will require
> more testing (but would JPRT not be enough? I think it would be
> sufficient for
> this change by and of itself to just test with JPRT) and push back the
> of this. So I am neutral on how or when this is done.
> -- ramki
More information about the hotspot-gc-dev