Request for review(M): 6627983: G1: Bad oop deference during marking
igor.veresov at oracle.com
Mon Feb 28 18:50:25 PST 2011
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
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.
> 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
>>>> 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
More information about the hotspot-compiler-dev