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

[Y. S. Ramakrishna]

Hi Bengt, John -- A couple of comments inline on some of the issues
mentioned by Bengt & Stefan in their review...

On 09/13/11 03:36, Bengt Rutisson wrote:
...
> This was a good explanation. Now we understand why it is called 
> is_alive_non_header. However, considering that we misunderstood it and 
> were not sure if it was required for completeness we think it would be 
> good to rename it and/or add comments that this is an optimization.
> 
> BTW, wouldn't ParallelOld be able to use this as well? And shouldn't 
> young generational collectors be able to filter all references out that 
> points to referents outside the collection set?

ParallelOld uses external live marks, and should use this mechanism as well,
if it is not already.

The young collectors are already covered by the existing filters (is gc marked
should test the same bit that forwarding sets, i think -- we should verify);
the non-header test was introduced out-of-line precisely for collectors that
did not have live marks in the header, as John explained.

...
> Interesting. Is there a CR to remove the ReferentBasedDiscovery policy?

(see previous email from me) I'll file one if one does not already exist. I'll check.


>>> 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?)

...
>>>
>>> - 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