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

[Igor Veresov]

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