Request for review(M): 6627983: G1: Bad oop deference during marking
igor.veresov at oracle.com
Thu Feb 24 18:02:46 PST 2011
On 2/24/11 5:27 PM, John Rose wrote:
> On Feb 24, 2011, at 4:49 PM, Igor Veresov wrote:
>> Just by code analysis. Did I miss something?
> I felt a slight doubt because there are tricky paths through the various subroutine layers in library_call. I see you converted "must_clear_dest" to "suppress_pre_barrier" along two paths. There's a path through generate_block_arraycopy which *almost* needs the flag also, but it allows the flag to default.
> To make the code more obviously correct, I suggest making the flag non-optional in library_call. And when the role of the flag changes, a line or two of code could mark the transition like this:
> bool use_pre_barrier = (bt == T_OBJECT); // usual case
> if (must_clear_dest) use_pre_barrier = false; // 6627983 suppress pre_barrier, if any
> foo(..., use_pre_barrier);
> 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? 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.
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.
> 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.
> -- John
More information about the hotspot-compiler-dev