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

[Igor Veresov]

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