RFR: 8069367: assert(_nextMarkBitMap->isMarked((HeapWord*) obj)) failed

[Kim Barrett]

Thomas,

Thanks for looking at this.

On Apr 13, 2015, at 11:46 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> - for whatever reason the unified diff is broken, only containing the
> TEST.groups and the new test changes.

Maybe this bug?
https://bugs.openjdk.java.net/browse/CODETOOLS-7901115

> - I would have preferred the refactoring to be separate from the actual
> change.
>
> - why is the test excluded from the hotspot_gc target?
>
> - please avoid reformatting indentation of parameters to something
> non-standard (e.g. in G1CollectedHeap::initialize()).

For these, see earlier reply to Bengt. 

> - I think the third paragraph is somewhat incomplete: the issue is not
> only the remaining remembered set entries, but that objects referenced
> from that object would need to be kept alive as well.

The first paragraph is about maintaining SATB invariants.  The third
paragraph is about JDK-8071913.  See reply to Bengt.

> - line 3474: I would prefer if the if does not start with the
> negation.  While this case is arguably the more common one, there does
> not seem to be any performance concern at this point. Reading
> negations is always harder than the regular form.

I understand and agree with the preference for a non-negated test.

However, I wrote it that way because I liked all of the alternatives I
could think of less.

I don't like pre-clearing the candidates set for the same reason the
old code avoided clearing its set when there were no candidates; why
waste time doing unnecessary work.  (A debug-only clear is unappealing
because that would be a side effect that is visible to non-debug code,
with the potential for debug and non-debug unintentionally having
different behavior.)

I don't like just re-ordering the clauses because it puts the else
clause a long way from the test.

A change I considered but didn't take was to re-order the clauses but
extract most of the present else body into a helper function.
(Specifically, I'm thinking the remset purge to the dirty card queue
would be a good cut point.)  I didn't do that because that kind of
code motion makes review harder.  I'd be fine with doing that as a
followup, or even as part of this change if that's preferable to
reviewers.

> Calculation of the hrm_index() could be factored out for both cases.

It could, but I don't think that's helpful in the present form.  If
the remset purge were moved to a helper function then it would make
more sense, with all uses being textually close together.