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

[Thomas Stuefe]

On Tue, 2 Mar 2021 00:55:16 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 one additional commit since the last revision:
> 
>   Using regular brackets in initializer list instead of curly brackets

Hi Misha,

> Hi Thomas,
> I do most of my JDK work in Java, hence may have missed knowing the restriction of using STL.
> I wonder if this restriction is only for source code, and not the tests. I did a quick search under "test/hotspot/gtest", and found many uses of std::, including the data structures. For instance, jfr/test_networkUtilization.cpp uses std::map, std::list and std::vector.

See my answer to Coleen. I think using the STL would have a number of repercussions which should be discussed before doing this step. 

About the patch itself:

I am not sure moving more and more test control down into gtest is a good thing. gtests have mostly be single threaded until now and could be run within a make. AFAIU gtests offer way less run control than jtreg does, e.g. you cannot disable the test without recompiling the hotspot (there is no ProblemList equivalent for gtest), you have no test groupings (e.g. to separate stress tests which need a whole machine for themselves) etc. Also, we will duplicate more and more thread control in C++.

I understand the wish to remove the test coding from the hotspot implementation. Its so ugly and should not live there. But just moving them to separate implementation files, possibly within a clearly marked "tests" folder, would be a first good step.

I will not block this if you have decided to go this way. Just wanted to understand the direction you guys plan to go with gtests in the future. If they get more complex and powerful we may need more control, eg the possibility to problemlist tests.

Cheers, Thomas

test/hotspot/gtest/concurrent_test_runner.inline.hpp line 27:

> 25: #define GTEST_CONCURRENT_TEST_RUNNER_INLINE_HPP
> 26: 
> 27: #include "threadHelper.inline.hpp"

Make sure you include all headers needed for this file. Includes should be self-contained, so pull everything they need (basically, you should be able to include it into an empty cpp file and it should build fine). You use Semaphore and some os::xxx functions, so you'd need at least os.hpp and wherever Semaphore lives.

test/hotspot/gtest/concurrent_test_runner.inline.hpp line 69:

> 67:     testDurationMillis(testDurationMillisArg) {}
> 68: 
> 69:   virtual ~ConcurrentTestRunner() {}

Do we derive from this class? And delete via base pointers? If not, I'd remove this.

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

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