Request for review(M): 6627983: G1: Bad oop deference during marking

[Vladimir Kozlov]

+     // Also, if this flag is set we make sure that arraycopy intracts properly
                                                                 ^^^
Vladimir

Igor Veresov wrote:
> John,
> 
> I'd like to rename must_clear_dest to dest_uninitialized and pass to 
> arraycopy instead of introducing additional variables. What do you think?
> 
> Here is the webrev:
> http://cr.openjdk.java.net/~iveresov/6627983/webrev.02
> 
> Thanks,
> igor
> 
> On 3/1/11 1:53 PM, Igor Veresov wrote:
>> On 3/1/11 1:08 PM, Tom Rodriguez wrote:
>>>
>>> On Mar 1, 2011, at 12:53 PM, Igor Veresov wrote:
>>>
>>>> On 3/1/11 12:23 PM, Vladimir Kozlov wrote:
>>>>> I have only comments about library_call.cpp:
>>>>>
>>>>> Why "false" in copy_to_clone()? It seems, you get double barriers in
>>>>> this method as well:
>>>>> 4085 /*dest_uninitialized*/false);
>>>>
>>>> It doesn't matter here, because we're treating it all as T_LONGs, so
>>>> there's going to be no barriers. But for consistency I suppose it
>>>> could be "true" here.
>>>>
>>>>>
>>>>>
>>>>> 4596 // so the the pre-barriers wouldn't peek into the old values. See
>>>>> CR 6627983.
>>>>> ^ 'the' 2 times ^ unitialized
>>>> Fixed. Thanks!
>>>>
>>>>> 4597 const bool& dest_uninitialized = must_clear_dest;
>>>>> ^ why reference?
>>>>>
>>>>
>>>> John wanted a separate variable to inform arraycopy of whether the
>>>> target is initialized. This variable should in fact follow the state
>>>> of the must_clear_dest, so I decided to make it a reference, so that
>>>> when must_clear_dest changes so does dest_uninitialized. If you don't
>>>> like that I can make it a regular variable.
>>>>
>>>> Although I would rather just used must_clear_dest for everything and
>>>> not replicate the state.
>>>
>>> no local variable references please. Just pass must_clear_dest or
>>> rename it to dest_uninitalized.
>>
>> Ok.
>>
>> igor
>>
>>>
>>> tom
>>>
>>>>
>>>>
>>>> Thanks,
>>>> igor
>>>>
>>>>
>>>>> The rest changes seems fine.
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>> Igor Veresov wrote:
>>>>>> John, all,
>>>>>>
>>>>>> I've updated the change, making corrections that John recommended.
>>>>>> Also, now the decision about what to do with the barrier is deferred
>>>>>> to the barrier emitting procedure. Thus, if a new pre-barrier is 
>>>>>> added
>>>>>> that would need to do something even in case if the destination array
>>>>>> is uninitialized it can be handled properly.
>>>>>>
>>>>>> New webrev: http://cr.openjdk.java.net/~iveresov/6627983/webrev.01
>>>>>>
>>>>>> Thanks,
>>>>>> igor
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2/25/11 8:25 PM, Igor Veresov wrote:
>>>>>>> I think my fix is not good enough for the case when we'll need
>>>>>>> prebarriers that will not require the previous value and these
>>>>>>> could be
>>>>>>> a reality in the future. So, it is generally incorrect to elide
>>>>>>> prebarriers but is only ok for a specific flavor. I'm working to
>>>>>>> alleviate this problem a little bit and will post the updated 
>>>>>>> version
>>>>>>> early next week. John, I will also address your recommendations.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> igor
>>>>>>>
>>>>>>>
>>>>>>> On 2/24/11 6:28 PM, John Rose wrote:
>>>>>>>> On Feb 24, 2011, at 6:02 PM, Igor Veresov wrote:
>>>>>>>>
>>>>>>>>> On 2/24/11 5:27 PM, John Rose wrote:
>>>>>>>>>> On Feb 24, 2011, at 4:49 PM, Igor Veresov wrote:
>>>>>>>>>>
>>>>>>>>>> If it is simply foo(..., !must_clear_dest), it becomes hard for
>>>>>>>>>> maintainers to see what the argument means.
>>>>>>>>>
>>>>>>>>> I can see your point but I just didn't want to introduce 
>>>>>>>>> additional
>>>>>>>>> clutter. For example generate_block_arraycopy() never requires
>>>>>>>>> barrier suppression, so why do we need to add an argument there?
>>>>>>>>
>>>>>>>> There's not a strong need. An extra "true" or "false" would be a
>>>>>>>> little clutter, but it would also hint to programmers what's
>>>>>>>> going on.
>>>>>>>> It's a esthetic call mostly...
>>>>>>>>
>>>>>>>> (Here's some meandering on the subject. While reading the code, 
>>>>>>>> if I
>>>>>>>> am tracing the flow from caller definition to callee definition,
>>>>>>>> when
>>>>>>>> arguments are explicit I can observe them directly. But if they are
>>>>>>>> defaulted, then I have to visit the callee's declaration as well. I
>>>>>>>> have to visit three locations in the source instead of two. I
>>>>>>>> personally find this awkward unless the meaning of the defaulted
>>>>>>>> argument is immediately and intuitively obvious. In this case the
>>>>>>>> meaning of the argument is subtly related to other parts of the
>>>>>>>> system.)
>>>>>>>>
>>>>>>>>> From the debugging standpoint it's way better to emit a barrier
>>>>>>>>> than
>>>>>>>>> not to. Missing barriers will be a nightmare to find. So, I'd
>>>>>>>>> rather
>>>>>>>>> have a barrier erroneously emitted in some path by default, which
>>>>>>>>> would make the marking crash almost immediately.
>>>>>>>>
>>>>>>>> Good point.
>>>>>>>>
>>>>>>>>> As for using !must_clear_dest as a gate, I thought it would more
>>>>>>>>> clearly convey to the reader the reason why the barrier is
>>>>>>>>> suppressed
>>>>>>>>> - because the dest array is not cleared and contains garbage, so
>>>>>>>>> the
>>>>>>>>> reader won't have to go back to see in which cases
>>>>>>>>> use_pre_barrier is
>>>>>>>>> set. Perhaps I should add more comments.
>>>>>>>>
>>>>>>>> The bug number would help a lot, I think. It's a subtle interaction
>>>>>>>> between (a) zeroing removal, (b) the arraycopy stubs, and (c) G1
>>>>>>>> invariants.
>>>>>>>>
>>>>>>>>>> In the stub generator the optional boolean is OK as is, 
>>>>>>>>>> although I
>>>>>>>>>> generally prefer optional booleans to default to 'false'. Given
>>>>>>>>>> the
>>>>>>>>>> way the bug is structured, I keep wanting to call it
>>>>>>>>>> 'suppress_pre_barrier', meaning "just this once, trust me when I
>>>>>>>>>> tell you not to emit the pre-barrier, if there is one."
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> That's pretty much equivalent. I don't have strong feelings about
>>>>>>>>> this and will change it if you think it's more readable.
>>>>>>>>
>>>>>>>> I do think it would be more readable. It could be just an old 
>>>>>>>> Lisper
>>>>>>>> bias, though: Optional arguments default to NULL, which is the 
>>>>>>>> false
>>>>>>>> value in Lisp.
>>>>>>>>
>>>>>>>> -- John
>>>>>>>
>>>>>>
>>>>
>>>
>>
>