RFR (S): 7080389: G1: refactor marking code in evacuation pause copy closures

[John Cuthbertson]

Wow. I certainly didn't think the changes were this controversial.

I don't want to change the template parameter name unless it's to remove 
the "forwardee" (to perhaps do_mark_obect[s], or should_mark_object[s]) 
because it's is no longer strictly true that its the forwarded object 
that is marked. Forwarded objects are now marked by the code in 
copy_to_survivor_space. But changing the comment to include that 
do_mark_forwardee is true during an initial mark pause for the root 
scanning closures is fine.

JohnC

On 08/19/11 09:44, Stefan Karlsson wrote:
> On 2011-08-19 18:23, Tony Printezis wrote:
>>
>>
>> On 08/19/2011 11:58 AM, Stefan Karlsson wrote:
>>>> Would you like better if John changes the comment to something like 
>>>> "we've been asked to mark the objects" and make sure the place 
>>>> where do_mark_forwardee is set to true describes why it is?
>>>
>>> Yes.
>>>
>>> I like the parameter name. The thing I'm having a problem with is 
>>> that the parameter name is "generic" in that sense that it doesn't 
>>> convey who's setting it to true. 
>>
>> Whoever wants to make sure every object visited by the closure is 
>> marked.
>
> I see that you're misunderstanding what I'm trying to say. I like the 
> parameter name, but I don't like the comments.
>
> StefanK
>
>>
>> Tony
>>
>>> But then we have the comment that goes on and reveals that 
>>> information anyway.
>>>
>>> StefanK
>>>
>>>>
>>>> Tony
>>>>
>>>> On 08/19/2011 10:43 AM, Stefan Karlsson wrote:
>>>>> On 08/19/2011 04:39 PM, Tony Printezis wrote:
>>>>>> You think "do_mark_forwardee" is generic?!?!?! It's very 
>>>>>> descriptive on what it does.
>>>>>
>>>>> Well, it's more generic than something like 
>>>>> do_mark_forwardee_during_initial_mark_root_scanning, which at 
>>>>> least to me, the comments imply. Anyways, you don't have to listen 
>>>>> to my arguments if you don't want to.
>>>>>
>>>>> StefanK
>>>>>
>>>>>>
>>>>>> Tony
>>>>>>
>>>>>> On 08/19/2011 10:35 AM, Stefan Karlsson wrote:
>>>>>>> Tony,
>>>>>>>
>>>>>>> On 08/19/2011 04:19 PM, Tony Printezis wrote:
>>>>>>>> Stefan,
>>>>>>>>
>>>>>>>> OK, good point. Maybe John can change the comment to something 
>>>>>>>> like "Need to mark the copied object if we're root scanning 
>>>>>>>> closure during initial mark, ....". Would this address your 
>>>>>>>> concern?
>>>>>>>
>>>>>>> I just don't see the reason for giving the parameter such a 
>>>>>>> "generic" name and then having comments about initial marking 
>>>>>>> root scan in the code whenever the parameter is used.
>>>>>>>
>>>>>>> StefanK
>>>>>>>
>>>>>>>>
>>>>>>>> Tony
>>>>>>>>
>>>>>>>> On 08/19/2011 10:01 AM, Stefan Karlsson wrote:
>>>>>>>>> But that's not what the comment says:
>>>>>>>>>
>>>>>>>>> 4339   // Need to mark the copied object if we're an initial
>>>>>>>>> 4340   // mark closure, or the object is already marked and
>>>>>>>>> 4341   // we need to preserve the mark.
>>>>>>>>> 4342   bool should_mark = do_mark_forwardee ||
>>>>>>>>> 4343       (_g1->mark_in_progress()&&  !_g1->is_obj_ill(obj));
>>>>>>>>>
>>>>>>>>> and
>>>>>>>>>
>>>>>>>>> 4357     // Object is not in collection set - if we're an 
>>>>>>>>> initial mark
>>>>>>>>> 4358     // closure then mark the object.
>>>>>>>>> 4359     if (do_mark_forwardee) {
>>>>>>>>>
>>>>>>>>> StefanK
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Tony
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>