RFR(S): 8159817: Convert FreeRegionList_test to GTest

Kim Barrett kim.barrett at oracle.com
Tue Aug 9 21:13:21 UTC 2016


> On Aug 5, 2016, at 12:01 PM, Kirill Zhaldybin <kirill.zhaldybin at oracle.com> wrote:
> 
> Dear all,
> 
> Could you please review this fix for JDK-8159817?
> 
> Since this test - freeRegionList - could only be run with G1 GC I added a new macro 
> TEST_OTHER_VM_WITH_FLAGS which runs a test in a new VM which is started with specified command line options.
> 
> WebRev: http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8159817/webrev.00/
> CR: https://bugs.openjdk.java.net/browse/JDK-8159817
> 
> Thank you.
> 
> Regards, Kirill

I'm uncomfortable with the new TEST_OTHER_VM_WITH_FLAGS.  I think this
may run into problems in conjunction with options passed in to the
main gtest execution.  Jesper and I had a bit of discussion about that
about a month ago; I thought he was going to file an RFE, but couldn't
find one, only a bit of the email exchange.

As an example, if the main test execution is invoked with
-XX:+UseXXXGC where XXX is not G1, then the TEST_OTHER_VM_WITH_FLAGS
for the new test will lead to problems because of multiple GCs being
requested for the child VM.  Just in general I'm not sure we have
facilities for (or even a design for) turning tests on or off based on
options, and I suspect we need that.

Also, I think the use of a child VM here is related to using
FreeRegionList::verify_list() in the test, without gtestifying it,
e.g. converting assert/guarantee to gtest assertions.  I don't think
this test actually needs (or even wants) an initialized VM, but
running it in the main VM and having verify_list() report failure
would kill the main test VM without allowing any other tests to run.
Of course, verify_list() is also used in normal code, so we might need
two versions of it.  But having two versions is messy unless there is
a single verify_list that is somehow parameterized for different
reporting mechanisms.

If we ignore those issues and just look at this change in isolation,
it seems mostly ok, though I have some issues with the implementation
of TEST_OTHER_VM_WITH_FLAGS.  But I'm wondering if conversion of this
test might be premature at this point, because we need some more
infrastructure.  OTOH, even if that's true, the attempt seems useful
in identifying infrastructure holes that might need filling.

Specific issues:

------------------------------------------------------------------------------ 
test/native/gc/g1/test_freeRegionList.cpp
  32 TEST_OTHER_VM_WITH_FLAGS(g1, freeRegionList, (char*) "-XX:+UseG1GC") {

Why is const being cast away on the flags string?  That either
shouldn't be needed, or probably indicates a bug in the underlying
infrastructure.

------------------------------------------------------------------------------ 
test/native/unittest.hpp

There are large chunks of TEST_OTHER_VM_WITH_FLAGS that are duplicates
of similar chunks in TEST_OTHER_VM.  It would be nice to factor those
chunks out into shared helpers.

[pre-existing] There are other moderately complicated and frequently
duplicated chunks in this file, with more occurrences added by
TEST_OTHER_VM_WITH_FLAGS.  For example, I see many occurrences of

  test_ ## category ## _ ## name ## _()

------------------------------------------------------------------------------




More information about the hotspot-gc-dev mailing list