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

[Stefan Karlsson]

On 08/19/2011 05:36 PM, Tony Printezis wrote:
> Of course I want to listen to what you have to say...
>
> I understand your argument but I hope you can also see where I'm 
> coming from. It's good to name the parameter with the action that it 
> does and set it when it's appropriate (in this case for some closures 
> during initial mark).

Yes, I like that part.

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