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

[Igor Veresov]

On 3/1/11 2:33 PM, Vladimir Kozlov wrote:
> + // Also, if this flag is set we make sure that arraycopy intracts
> properly
> ^^^

Thanks.

igor

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