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

[Tony Printezis]

Bengt,

Please consider the following. John identified this small cleanup while 
working on his changeset. If he cannot do it because he's not otherwise 
touching that file, when will it be done? Should he add it to a to-do 
list for the next time he touches thread.cpp (which might not be any 
time soon, given that it's not a file we usually touch)? Or should he 
send a blanket e-mail saying "if anyone is changing thread.cpp please 
add this small cleanup to your changeset"? My gut feeling is that, if he 
cannot do the cleanup as part of the changeset he's currently working 
on, it will not be done any time soon, which is unfortunate. So, IMHO at 
least, incorporating small cleanups like this (even if they are very 
minor) is more important than not touching files not otherwise updated 
by the fix for a CR and we should be able to get them in with very low 
overhead.

Tony

Bengt Rutisson wrote:
> 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
>