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

[Tony Printezis]

John,

Good observation re: do_mark_forwardee not being quite accurate any more 
(the same for the mark_forwardee() method too). I'd be OK with them 
getting renamed to do_mark_object().

Tony

On 08/19/2011 01:19 PM, John Cuthbertson wrote:
> 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
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>
>