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

[Jon Masamitsu]

Ok.  I'm good with both your responses.

Jon
On 11/6/2013 8:52 AM, Thomas Schatzl wrote:
> 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
>>>>
>