RFR(S): 8159817: Convert FreeRegionList_test to GTest

Kirill Zhaldybin kirill.zhaldybin at oracle.com
Wed Oct 19 21:19:25 UTC 2016


Dear all, Kim,

After the discussion on Gtest and a few fixes which improved 
assert/guarantee behavior if they were triggered in gtest I made a new 
version of this fix.

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

Changes:
1. Instead of new macro I added a check which prevents the test to be 
run if G1 is not the current GC
2. Since verify_list is uses guarantee it could be considered safe to be 
used in gtests.

Could you please let me know your opinion?

Thank you.

Regards, Kirill

On 10.08.2016 20:11, Kirill Zhaldybin wrote:
> 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.
>>
>> ------------------------------------------------------------------------------
>>
>>
>




More information about the hotspot-gc-dev mailing list