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

[Vladimir Kozlov]

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


4596   // so the the pre-barriers wouldn't peek into the old values. See CR 6627983.
                  ^ 'the' 2 times                        ^ unitialized
4597   const bool& dest_uninitialized = must_clear_dest;
                  ^ why reference?

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