RFR(S): 8159817: Convert FreeRegionList_test to GTest

[Kirill Zhaldybin]

Kim,

Thank you for reviewing the fix.

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

Changes:
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 
TEST_VM_ASSERT and TEST_VM_ASSERT_MSG
>
> [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.
>
> ------------------------------------------------------------------------------
>