RFR: 8213269: convert test/hotspot/jtreg/runtime/memory/RunUnitTestsConcurrently to gtest

[Igor Ignatyev]

On Fri, 5 Feb 2021 20:35:23 GMT, Mikhailo Seledtsov <mseledtsov at openjdk.org> wrote:

> This is a preliminary review. I would like to get the initial feedback before I proceed with conversion of the remaining tests.
> 
> Here is what I did so far:
>   - created a UnitTestThread and a main test runner, based on gtests with similar needs
>   - moved the original code from HotSpot internals (so called hotspot internal tests: src/hotspot/share/memory/virtualspace.cpp)
>     to the newly created gtest while wrapping it into a TestReservedSpace class. I did not change the code of the test.
>   - removed invocations from whitebox.cpp
> 
> Testing:
>   - ran GTestWrapper on usual platforms - All PASS
>   - ensured that ReservedSpaceConcurrent is in the logs and passed
> 
> After gathering the feedback my plan is:
> Plan:
>   - move the remaining internal Memory/VirtualSpace tests into a gTest
>     - I am thinking about using separate files for each test
>   - create a common file for UnitTestThread and MultiThreadTestRunner to reuse the code

hi Misha,

I haven't finished (pre)reviewing it yet, however, I have found a few things that, I belive, should be changed.

test/hotspot/gtest/runtime/test_reservedSpaceConcurrent.cpp line 1:

> 1: /*

test files should follow the same naming scheme as the product files which contained tested functionality, so in this case, it should be `test/hotspot/gtest/runtime/memory/test_virtualspace.cpp`

test/hotspot/gtest/runtime/test_reservedSpaceConcurrent.cpp line 30:

> 28: 
> 29: #define TEST_THREAD_COUNT 10 // TODO: update to original value of 30
> 30: #define TEST_DURATION 15000 // milliSeconds

it's 2021, there is (almost) no good reasons to use `#define` to define constants.

test/hotspot/gtest/runtime/test_reservedSpaceConcurrent.cpp line 208:

> 206:   void run_unit_test() {
> 207:     TestReservedSpace::test_reserved_space();
> 208:   }

what's the point of having this extra member function? can't we just call `TestReservedSpace::test_reserved_space` directly from main_run?

test/hotspot/gtest/runtime/test_reservedSpaceConcurrent.cpp line 46:

> 44:   static void release_memory_for_test(ReservedSpace rs) {
> 45:     if (rs.special()) {
> 46:       guarantee(os::release_memory_special(rs.base(), rs.size()), "Shouldn't fail");

you shouldn't use hotspot's `gurantee/assert`, and use gtest provided `ASSERT`/`EXPECT` instead.

-------------

Changes requested by iignatyev (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2436