RFR(M/L): 6484982: G1: process references during evacuation pauses
Bengt Rutisson
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:
>>>> thread.cpp
>>>> - 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
> although
> this fixes the formatting, that such formatting changes should not be
> part
> 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
my opinion.
Bengt
>
> ...
>>>>
>>>> - 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.
>>
>> OK.
>>
>
> Not now, but i think we should try and fix this in the future. I don't
> completely
> 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.)
>
>>>>
>>>> g1CollectedHeap.cpp
>>>> - 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");
>>>> ref_processor_stw()->verify_no_references_recorded();
>>>>
>>> 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
> integration
> of this. So I am neutral on how or when this is done.
>
> -- ramki
More information about the hotspot-gc-dev
mailing list