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

[Vladimir Kozlov]

Igor,

Should you also check G1DisablePreBarrier flag which is currently used only here?:

src/share/vm/c1/c1_LIRGenerator.cpp:  if (G1DisablePreBarrier) return;

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