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

[Tom Rodriguez]

I don't know why it's there but you can delete it if you want.

tom

On Mar 1, 2011, at 3:18 PM, Igor Veresov wrote:

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