RFR (XS): 8021823 G1: Concurrent marking crashes with -XX:ObjectAlignmentInBytes>=32 in 64bit VMs (patch for hs24+hs25)

[Thomas Schatzl]

Hi,

On Thu, 2013-08-01 at 07:49 +0200, Bengt Rutisson wrote:
> 
> Hi Thomas,
> 
> The code change looks good.
> 
> One question about the test. I know that the current failure was with
> G1, but all of our GC should work with all the supported
> ObjectAlignments, right?

I agree.

> Why do we only want to test this with G1? Maybe we could just remove "
>  -XX:+UseG1GC" from the command line and get testing for all GCs? In

This test has been intended for G1 only from the start, and only for
checking the concurrent marking.

For other collectors, with the need to check all sub-collectors like
defnew and serial old and their combinations, the test would be much
more complicated than it is currently.

I.e. for every valid combination of collectors we have the test would
need to set up the heap properly first, and then trigger a collection of
that particular type. Particularly you also want to check the backup
collectors (like G1 full "serial" and CMS full "serial" too).

It is not just enabling the same test for the other collectors
unfortunately.

It would be really nice if we had at least internally some extension of
System.gc() or just an internal method where you could specify the type
of collection you want (young only, eventually mixed+young only,
concurrent, "full" backup), and a way to wait for completion of that if
necessary. With some additional info about if e.g. some objects were
promoted or not etc to e.g. see that the promotion code has been tested.

Maybe there is already? Any other ideas?

This seems to be out of scope here for this simple fix; I could file
some RFEs for that (tests for ObjectAlignmentInBytes and some small test
api to be able to have more control on the GCs and get some information
on the gc behavior back).

>  that case I guess you probably don't need to use the ProcessTools
> either. You could have several @run commands instead.

With @run commands you have the additional problem of the test runner
then trying to unconditionally run the test also on VMs not supporting
particular GCs. -XX:+IgnoreUnrecognizedVMFlags or so does not work with
gc options afair.

If you do not specify the gc, you can never be sure that a particular GC
combination is run or not, which may give unpleasant surprises at some
point. As you may have noticed I do not really like the "somebody will
eventually run all combinations" approach to testing - this fuzzing type
testing is a good addition, but should not be the only kind (imo).

I would like to propose the following addition to this test though: when
looking at how you recently retrieved the value of G1HeapRegionSize from
the VM, I had an idea how to detect the support of the g1 collector:
g1_globals.hpp does not get compiled in for VMs not supporting them, so
simply checking for its availability solves that.

See the new webrevs at
hs24: http://cr.openjdk.java.net/~tschatzl/8021823/webrev.3.hs24/
hs25: http://cr.openjdk.java.net/~tschatzl/8021823/webrev.3.hs25/

What do you think?

Thanks for your input,
Thomas