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

[Thomas Schatzl]

Hi,

On Mon, 2015-04-13 at 22:28 -0400, Kim Barrett wrote:
> 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

Probably. I made good experience with the workaround to delete any
existing webrev directory beforehand.

> > - 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. 

Thanks.

> > - 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.

Thanks for clarification.

> > - 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.)

Even keeping the clearing always enabled will not impact performance too
much because it will compile down to a memset call for a "small" memory
area. (I know the previous version has been written by me).

> 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.

Instead of explicit add/remove calls one could pass the value of
humongous_region_is_candidate() directly to a "set"-like method.

I think however the distance between the test and the else clause will
not be of concern as soon as the code to flush the small remembered sets
to the card table has been moved out.

> > 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.

It's fine for me to do that sort of refactoring in a separate changeset.

Thanks,
  Thomas