RFR(XS) : 8177708 : Convert TestReserveMemorySpecial_test to Gtest

[Gerard Ziemski]

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
>> 
>