RFR(XS) : 8177708 : Convert TestReserveMemorySpecial_test to Gtest

[Igor Ignatyev]

Gerard,

#1. this assert is kinda contract of the function. test_reserve_memory_special_shm is an aux test function which works iif UseSHM is true. invoking it when UseSHM is false is a test error, and I'd prefer not to hide such errors and get a test error. 'if (!$X) { return }' construction is used only in actual tests and is current workaround to express things similar to @requiers in jtreg.

#2. I did the testing appropriated for the made changes: all gtest tests (including new ones) on all "regular" platforms w/ precompiled header enabled and disabled. I've also run mach5 tier1 job just in case, running any other tiers won't provide us w/ any extra coverage.

I have to say though, as these tests depend on host configuration (thru vm flags values), getting passed status doesn't mean much as all the test tasks could have ended up on hosts which don't support large pages. 

Thanks,
-- Igor

> On Nov 1, 2018, at 10:58 AM, Gerard Ziemski <gerard.ziemski at oracle.com> wrote:
> 
> hi Igor,
> 
> Nice cleanup.
> 
> I tried to follow the original code and the changes/cleanups you did, and it seems OK to me, even if the new code seems a bit less verbal. I do have 2 questions though:
> 
> #1 In test/hotspot/gtest/runtime/test_os_linux.cpp we have:
> 
>  83 static void test_reserve_memory_special_shm(size_t size, size_t alignment) { 
>  84 ASSERT_TRUE(UseSHM) << "must be used only when UseSHM is true"; 
> 
> But in src/hotspot/os/linux/os_linux.cpp we have:
> 
> 6110  static void test_reserve_memory_special_shm(size_t size, size_t alignment) {
> 6111    if (!UseSHM) {
> 6112      return;
> 6113    }
> 
> Why do we bother with “ASSERT_TRUE” here, when the original code didn’t, and when other checks for "UseHugeTLBFS" simply silently return? I’d prefer we either use ASSERT_TRUE for all the flags here (if it doesn’t hurt the test), or leave it as in the original code.
> 
> #2 Did you run any Mach5 tiers to test this change?
> 
> 
> cheers
> 
>> On Nov 1, 2018, at 12:11 PM, Igor Ignatyev <igor.ignatyev at oracle.com> wrote:
>> 
>> Hi Gerard,
>> 
>> #1. As I mentioned earlier and you can see from your grep results, this test is still called WhiteBox::runMemoryUnitTests which is used in runtime/memory/RunUnitTestsConcurrently to run this and a few other tests multiple times in several concurrent threads. runtime/memory/RunUnitTestsConcurrently will be converted to gtest separately, and this conversion will also include removing of all these tests.
>> 
>> #2. only linux and windows version of TestReserveMemorySpecial_test aren't trivial, the rest are just empty.
>> 
>> #3. semantically linux gtest is doing the same as TestReserveMemorySpecial in  open/src/hotspot/os/linux/os_linux.cpp, and windows gtest --  open/src/hotspot/os/linux/os_windows.cpp. yes, there are some differences caused by difference in "frameworks" and/or aimed to improve the tests, e.g. changes in how tests are split, error/trace message are printed, etc.
>> 
>> Thanks,
>> -- Igor 
>> 
>>> On Nov 1, 2018, at 8:44 AM, Gerard Ziemski <gerard.ziemski at oracle.com> wrote:
>>> 
>>> hi Igor,
>>> 
>>> I took a look at the webrev, but I think I’m missing the big picture. If I grep for TestReserveMemorySpecial I get:
>>> 
>>> # grep -rn TestReserveMemorySpecial
>>> open/src/hotspot/os/bsd/os_bsd.cpp:3778:void TestReserveMemorySpecial_test() {
>>> open/src/hotspot/os/linux/os_linux.cpp:5967:class TestReserveMemorySpecial : AllStatic {
>>> open/src/hotspot/os/linux/os_linux.cpp:6146:void TestReserveMemorySpecial_test() {
>>> open/src/hotspot/os/linux/os_linux.cpp:6147:  TestReserveMemorySpecial::test();
>>> open/src/hotspot/os/linux/os_linux.hpp:36:  friend class TestReserveMemorySpecial;
>>> open/src/hotspot/os/windows/os_windows.cpp:5561:void TestReserveMemorySpecial_test() {
>>> open/src/hotspot/os/solaris/os_solaris.cpp:5405:void TestReserveMemorySpecial_test() {
>>> open/src/hotspot/os/aix/os_aix.cpp:4321:void TestReserveMemorySpecial_test() {
>>> open/src/hotspot/share/prims/whitebox.cpp:231:void TestReserveMemorySpecial_test();
>>> open/src/hotspot/share/prims/whitebox.cpp:239:  TestReserveMemorySpecial_test();
>>> open/src/hotspot/share/utilities/internalVMTests.cpp:45:  run_unit_test(TestReserveMemorySpecial_test);
>>> 
>>> #1 Wouldn’t you want to delete TestReserveMemorySpecial class from the platform files (ex. open/src/hotspot/os/linux/os_linux.cpp, etc?) now that we have dedicated gtest files?
>>> 
>>> #2 Also, I only see linux and windows gtests, what about the ones for Mac and Solaris, now that you removed the test from internalVMTests.cpp?
>>> 
>>> #3 Are the linux, windows gtest files what TestReserveMemorySpecial class is/was in open/src/hotspot/os/linux/os_linux.cpp, open/src/hotspot/os/windows/os_windows.cpp? On cursory look I see some differences.
>>> 
>>> 
>>> cheers
>>> 
>>>> On Oct 22, 2018, at 1:36 PM, Igor Ignatyev <igor.ignatyev at oracle.com> wrote:
>>>> 
>>>> http://cr.openjdk.java.net/~iignatyev//8177708/webrev.00/index.html
>>>>> 331 lines changed: 330 ins; 1 del; 0 mod; 
>>>> Hi all,
>>>> 
>>>> could you please review this small and trivial patch which converts TestReserveMemorySpecial_test tests to Gtest? the old tests[1,2] haven't been removed as they are used by WhiteBox::runMemoryUnitTests they will be removed when the rest tests are converted.
>>>> 
>>>> webrev: http://cr.openjdk.java.net/~iignatyev//8177708/webrev.00/index.html
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8177708
>>>> [1] http://hg.openjdk.java.net/jdk/jdk/file/tip/src/hotspot/os/linux/os_linux.cpp#l5969
>>>> [2] http://hg.openjdk.java.net/jdk/jdk/file/tip/src/hotspot/os/windows/os_windows.cpp#l5564
>>>> 
>>>> Thanks,
>>>> -- Igor
>>> 
>> 
>