RFR(M/L): 6484982: G1: process references during evacuation pauses

[Bengt Rutisson]

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