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

[Igor Veresov]

On 3/1/11 2:59 PM, Vladimir Kozlov wrote:
> Igor,
>
> Should you also check G1DisablePreBarrier flag which is currently used
> only here?:
>
> src/share/vm/c1/c1_LIRGenerator.cpp: if (G1DisablePreBarrier) return;
>

Hm, it seems like a relatively meaningless C1 debugging flag (there's 
also G1DisablePostBarrier). It can only be useful just to check C1's 
output? G1 won't work without the barriers.

igor

> Vladimir
>
> Igor Veresov wrote:
>> 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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>