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

[Thomas Stuefe]

On Thu, 4 Mar 2021 02:44:15 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
>
> Mikhailo Seledtsov has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Adding os_ prefix to concurrent virtual space tests
>  - Using os:malloc instead of std::vector

Hi Misha,

two small remarks. As I wrote yesterday, I think this is a good cleanup despite my doubts. 

I don't see any GAs running. Can you enable them please? Under "Checks" we should see that all platforms build; also, since the gtests are part of tier1, they run too and we see that they work in all configurations. (eg like this: https://github.com/openjdk/jdk/pull/2751/checks)

You may have to enable github actions if you have never done so and this is your first work in your personal jdk fork (see "Actions" tab under your repo).

Cheers, Thomas

test/hotspot/gtest/concurrentTestRunner.inline.hpp line 76:

> 74:     size_t sz = sizeof(UnitTestThread*) * nrOfThreads;
> 75:     UnitTestThread** t = (UnitTestThread**) os::malloc(sz, mtInternal);
> 76:     memset((void*)t, 0, sz);

The memset is not needed.

And if you wanted you could shorten this to one line using:
UnitTestThread** t = NEW_C_HEAP_ARRAY(UnitTestThread*, nrOfThreads, mtInternal);
which would take care of size calculation and the cast for you. Also, it gives you automatic OOM handling should this ever be a problem.

If you do that, aesthetically it would make sense to change the 'os::free' below to: 'FREE_C_HEAP_ARRAY(UnitTestThread**, t);` though its just a wrapper around os::free.

And you'd need to include memory/allocation.hpp.

If you keep using os::malloc, which would be fine too, you need runtime/os.hpp.

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

Changes requested by stuefe (Reviewer).

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