RFR(XS) : 8177711 : Convert TestVirtualSpaceNode_test to GTest

[Igor Ignatyev]

Hi Thomas,

thanks a lot for looking at this. (un)fortunately, I pushed the fix, before receiving your email ;). anyhow here is the answers to your comments:

> MetachunkRemover seems to be nowhere used, can this be removed?
yes, I don't know how I forgot about it.

> does this compile with non-pch? Test file seems to miss some headers,
> e.g. virtualSpaceNode.hpp. Not sure if they come in implicitly.
yes, it compiles w/o pch. virtualSpaceList.hpp includes virtualSpaceNode.hpp, would you prefer to see all headers included explicitly?

> all_vsn_is_committed_half_is_used_by_chunks - can be completely removed IMHO.
> But still, bottomline, I'd rather remove the test. ...
there is 8212932 to clean up or remove this test, I've added your comments there, and will leave it up to runtime folks to decide what to do w/ this test. I have a feeling that some its parts are still valuable (but might require some refinement), that's why I decided to port the test so it at least won't be forgotten as a dead code. 

Cheers,
-- Igor

> On Oct 31, 2018, at 11:32 PM, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
> 
> Hi Igor,
> 
> Thanks for taking this on! This has been long overdue.
> 
> That said, with this particular test, I feel that since 8198423 its
> usefulness is greatly diminished, and there is a bit of a danger of
> getting it wrong, messing up global structures in metaspace and
> affecting follow up tests. Metaspace is not (anymore) designed to be
> used on this isolated level.
> 
> I really thought long and hard about this. Unfortunately, this test
> cannot be done in an isolated fashion anymore without affecting global
> metaspace - sure you encountered that yourself, hence the terrible
> (sorry :-) ChunkManagerRestorer workaround. And I do not see a better
> way to do this.
> 
> All these tests do now is to ensure that a particular notion of chunk
> geometry and allocation behavior is adhered to - stuff which may
> change anyway since it is implementation detail. I do not see any
> really worthwhile tests. And the fact that since 8198423 any
> allocation from a VirtualSpaceNode can cause allocation of padding
> chunks, which get added to the global freelist and hence change global
> state, makes matters quite complicated.
> 
> If you want to get ahead with this change, here some remarks:
> 
> ----
> 
> all_vsn_is_committed_half_is_used_by_chunks - can be completely removed IMHO.
> 
> ----
> 
> MetachunkRemover seems to be nowhere used, can this be removed?
> 
> ----
> 
> does this compile with non-pch? Test file seems to miss some headers,
> e.g. virtualSpaceNode.hpp. Not sure if they come in implicitly.
> 
> ----
> 
> But still, bottomline, I'd rather remove the test. When doing 8198423,
> I added a new gtest to make up for the now-teethless VSN gtest, see
> test_metaspace_allocation.cpp. There, I still stress Metaspace but a
> layer higher, using the same code paths the rest of the code uses when
> allocating metaspace.
> 
> Kind Regards, Thomas
> On Wed, Oct 24, 2018 at 8:50 PM Igor Ignatyev <igor.ignatyev at oracle.com> wrote:
>> 
>> http://cr.openjdk.java.net/~iignatyev//8177711/webrev.00/index.html
>>> 405 lines changed: 244 ins; 159 del; 2 mod;
>> 
>> 
>> Hi all,
>> 
>> could you please review this small and straightforward patch which converts TestVirtualSpaceNode_test into GTest? TestVirtualSpaceNode_test have been removed from executed by 8198423[1] and some of its asserts aren't true any more. I have them commented them out, and filed 8212932[2] to clean them up.
>> 
>> webrev: http://cr.openjdk.java.net/~iignatyev//8177711/webrev.00/index.html
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8177711
>> 
>> Thanks,
>> -- Igor
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8198423
>> [2] https://bugs.openjdk.java.net/browse/JDK-8212932
>>