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

[Igor Veresov]

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