RFR (XS): 8027756: assert(!hr->isHumongous()) failed: code root in humongous region? (P2-JDK8!)

[Thomas Schatzl]

Hi Jon,

On Tue, 2013-11-05 at 20:11 -0800, Jon Masamitsu wrote:
> Thomas,
> 
> I should also have said that the changes look good.

Thanks for the review - comments inline.

> 
> On 11/5/2013 4:00 PM, Jon Masamitsu wrote:
> > Thomas,
> >
> > Do you think there would be value in printing out the
> > region address in the cases where the assertion fails?
> 
> Your call as to whether it's worth printing the heap region.

I fixed up the assertions as part of Tao's request to change them.

> 
> >
> > Does your new test case fail with both the client and
> > server compilesr?
> 
> Question relates to whether you want to specify the JIT
> used when running the test.

I changed the test to try both server and client compilers. Both failed
btw.

New webrev at:
http://cr.openjdk.java.net/~tschatzl/8027756/webrev.1

Thanks,
Thomas

> >
> > On 11/5/2013 5:46 AM, Thomas Schatzl wrote:
> >> Hi all,
> >>
> >>    can I get quick reviews for the following small changes?
> >>
> >> The patchset fixes wrong assertions that assume that references to
> >> humongous/large objects cannot be embedded into the code stream. They
> >> can.
> >>
> >> This is not true, and in the failing (existing) test case just that
> >> happens.
> >>
> >> The fix is to just change all these wrong assertions to check whether
> >> the region we want to add the code root to is a humongous continuation:
> >> that really should not occur as they do not contain an object header.
> >>
> >> Note that there is a similar assertions with the same conditions in e.g.
> >> MigrateCodeRootsHeapRegionClosure::doHeapRegion() - however this one
> >> checks a different situation: there should be no migration of code roots
> >> because (currently) large objects can never move.
> >>
> >> The original reasons for adding this assertion, and why this assertion
> >> only triggers just now (the code has been in for a few months) are
> >> unknown. I will try to find out what change (possibly in the compiler)
> >> triggered this.
> >>
> >> The changeset also adds a new test in addition to the original one that
> >> particularly targets most reasons why the assertion failed before:
> >>   - when adding a new code root to a humongous region
> >>   - when removing a new code root to a humongous region
> >>   - during verification (does anybody know why
> >> G1VerifyHeapRegionCodeRoots is false by default?)
> >>   - strong code roots root marking (in anticipation of class unloading
> >> during concurrent marking)
> >>
> >> bugs.openjdk
> >> https://bugs.openjdk.java.net/browse/JDK-8027756
> >>
> >> Webrev
> >> http://cr.openjdk.java.net/~tschatzl/8027756/webrev/
> >>
> >> Testing:
> >> jprt, manual invocation of the failing test (verifying that without the
> >> change the assertion triggers, and does not with the patch), new test
> >>
> >> Thanks,
> >>    Thomas
> >>
> >
>