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

[Thomas Schatzl]

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