RFR(S): 8159817: Convert FreeRegionList_test to GTest

Kirill Zhaldybin kirill.zhaldybin at oracle.com
Wed Aug 10 17:11:23 UTC 2016


Thank you for reviewing the fix.

Here are new WebRev:
WebRev: http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8159817/webrev.02/

1. Moved code which creates child function to a separate macro, slightly 
improved error output and made 2 small helper macros.
2. Deleted (char*) which actually was not needed.

Could you please also read comments inline?

Thank you.

Regards, Kirill

On 10.08.2016 00:13, Kim Barrett wrote:
>> 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.
TEST_OTHER_VM_WITH_FLAGS rewrites _JAVA_OPTIONS without conjunction with 
existing ones so we cannot get two GCs specified.
As far as I understand from this - 
https://bugs.openjdk.java.net/browse/JDK-8158106 - gtests are not 
supposed to have any external flags.
> 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.
I did not do deep investigation but G1RegionToSpaceMapper::create_mapper 
fails if the test runs with SerialGC
> 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.
I did not find a good solution with verify_list().
First I tried to duplicate its functionality outside of FreeRegionList 
but since the verification process depends on private FreeRegionList 
variables it did not look like a possible solution.
Having parametrized version of verify_list() could require dependencies 
on gtest in FreeRegionList which looked  not very good too.
Since the test runs in separate VM and does not impact others I thought 
that we could leave verify_list() as is so far.
> 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.
Fixed, thank you for noticing this.
> ------------------------------------------------------------------------------
> 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.
I moved child function to separate macros and also used it in 
> [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 ## _()
Well, we could change it to some macro but it will be still something 
like TEST_FUNCTION(category, name). Do you think it worth it?

Thank you again.
> ------------------------------------------------------------------------------

More information about the hotspot-gc-dev mailing list