RFR: 8069367: assert(_nextMarkBitMap->isMarked((HeapWord*) obj)) failed
Thomas Schatzl
thomas.schatzl at oracle.com
Wed Mar 11 09:50:48 UTC 2015
Hi,
On Tue, 2015-03-10 at 19:23 -0400, Kim Barrett wrote:
> On Mar 9, 2015, at 5:52 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> >
> > Hi Kim,
> >
[...]
> > - concurrentMark.hpp: the _stale_humongous_queue_entries should be a
> > size_t. Sangheon has reviews for the patch that changes the other
> > related members from int's already. So maybe wait until it has been
> > pushed for posting the new webrev.
>
> Yes, I saw that review request. Looks like it’s been pushed.
Yes :)
>
> > - concurrentMark.cpp: what do you think about refactoring the
> >
> > is_stale_humongous_queue_entry(obj) {
> > // ignore
> > } else {
> > // do some asserts
> > scan_object();
> > }
> >
> > into an extra method? This would allow unification of the asserts too.
> > The "assert(...->is_marked(obj), ...)" is duplicated in
> > CMTask::do_marking_step(), scan_object() already does that. Also the
> > master_free_list assert could be moved into the scan_object() method I
> > think. I cannot see a case where this would not be the case.
>
> I was trying to minimize the changes, probably overly so. I’ve rearranged
> the asserts a bit and added a helper function. Note that it does not
> directly include the code for is_stale_… and eliminate that predicate;
> it turns out there’s another place that needs that predicate, in
> VerifyNoCSetOopsClosure.
Okay.
> > Also I would personally prefer to switch the if- and else- parts of that
> > (invert the condition). When reading if-statements, I somewhat expect
> > the far common case of execution to not be put into the else-part. It's
> > your call.
>
> I see it as a filter with the base case being the scan. Writing it that way also
> avoids needing to negate the predicate result, which I find easier to read.
> This *is* a place where I might wish for an UNLIKELY-style macro.
I am not insisting on changing this.
I did some experiments with a LIKELY/UNLIKELY macro just recently (for
8060025) on gcc, and I was only able to make performance worse (in the
memory allocation part of the code).
This mechanism seems way too brittle to be really useful: also
considering that it most likely requires regular maintenance.
> > Please add the test case used to reproduce that explicitly enables eager
> > reclamation. While failure without the change is not 100% reproducable,
> > it seems good enough.
>
> It was somewhat difficult to produce, but I’ve now got a reduced test case that
> usually fails pretty quickly on two quite different machine configurations. It will
> be included in the next webrev.
Great :) Looking forward to it.
Thanks,
Thomas
More information about the hotspot-gc-dev
mailing list