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

[Igor Veresov]

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
>