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

[Tao Mao]

Hi Thomas,

Thank you for a very informative response.

Looks good. Ship it!

Tao

On 11/6/13 12:12 PM, Thomas Schatzl wrote:
> Hi,
>
> On Wed, 2013-11-06 at 11:38 -0800, Tao Mao wrote:
>> Hi Thomas,
>>
>> The code changes look good.
>>
>> One question about the test:
>> How would the old code before your change fail the test?
>    the assertions that failed asserted on when the region we wanted to
> add a code cache root to was a humongous region. I.e. when the compiler
> embeds a reference to an oop into the code stream, it notifies the heap
> about this. In the process of this notification, G1 adds that nmethod to
> the region the oop starts to it's code cache roots.
>
> This is sort of remembered set that contains nmethods as roots for that
> region.
>
> When evacuating a particular region we use its nmethod remembered set
> (code cache roots) just like any other remembered set. This is a quick
> way to find the actually relevant nmethods, i.e. during gc this is much
> cheaper than iterating over the entire code cache looking for these
> references.
>
> That the code references a humongous object, is a valid occurrence for
> humongous objects/regions, as code may embed references to humongous
> (start) regions just like any other object.
>
> For whatever reason this went unnoticed for two months - I guess some
> recent compiler changes triggered that.
>
> This change changes the asserts from any kind of humongous region to
> just the continuations of humongous regions: humongous continuation
> regions do not contain an object start, and as mentioned before, we only
> ever add the nmethod to the region where the object starts in, so no
> nmethods should be added in continuations of humongous regions.
>
> The test tries to verify all these asserts:
> - when adding an nmethod to a region's remembered set, i.e. when
> generated code embeds a reference to a humongous object. Happens during
> compilation and on-stack-replacement of the loop that initializes the
> arrays in the code I guess.
> - during verification (i.e. at the System.gc() calls in the test), when
> the remembered set is checked for validity
> - and during removal of an nmethod from that remembered set, i.e. after
> code is unloaded (the deoptimizeAll() call) and has been swept. The
> Thread.sleep() in the test waits for the code sweeper to wake up and
> notice that there are methods to remove from the cache. The VM
> parameters for the forked VM in the test contains appropriate command
> line options.
> - there is a fourth case that the test code executes the asserts, and
> that is during an initial mark when class unloading is enabled.
> Admittedly, since G1 does not do class unloading after concurrent
> marking yet, this code is not exercised yet, but will be. The test
> simply sets IHOP to zero so that any young gc will be an initial mark
> where this is done :)
>
> To test whether these three test cases would trigger, I simply
> successively fixed one assert after another, and each time checking that
> the correct assert would be triggered at the expected place in the test.
>
> Other asserts in the migrate_* methods in the change basically check
> that humongous objects are not in the collection set. "Migration" of
> code roots is similar to remembered set update after region evacuation.
> Since we never put humongous objects into the collection set to evacuate
> them, this code path will and did not trigger before with the old code
> too. The test does not check this situation either. It's probably
> somewhat tricky to make the GC move humongous objects in a reproducable
> way.
> Testing must be added, and these asserts changed with any code that
> changes the evacuation policy for humongous objects (at that point a
> *ton* of similar asserts will trigger, so a few more do not hurt :)
>
> Hth,
> Thomas
>
>> Thanks.
>> Tao
>>
>> On 11/6/13 8:49 AM, Thomas Schatzl wrote:
>>> Hi,
>>>
>>> On Tue, 2013-11-05 at 10:49 -0800, Tao Mao wrote:
>>>> Hi Thomas,
>>>>
>>>> Can you make the assertion message from a question to a statement? It'll
>>>> help future readers understand it.
>>>>
>>>> say, in g1CollectedHeap.cpp
>>>>
>>>> - assert(!hr->continuesHumongous(), "code root in continuation of
>>>> humongous region?");
>>>> + assert(!hr->continuesHumongous(), "code root should not attached to a
>>>> humonguous continuation");
>>>>
>>>> There may be a couple of other similar ones.
>>> Done for all related assertions. There are more, but I did not think
>>> changing them fit into this changeset.
>>>
>>> See new webrev at http://cr.openjdk.java.net/~tschatzl/8027756/webrev.1/
>>>
>>> Also changed the test to try both server and client compilers (e.g.
>>> -client and -server).
>>>
>>> Thanks for the review,
>>> Thomas
>>>
>>>
>