RFR: 8292679: Simplify thread creation in gtest and port 2 tests to new way

[Thomas Stuefe]

On Mon, 22 Aug 2022 17:06:51 GMT, Johan Sjölén <duke at openjdk.org> wrote:

>> test/hotspot/gtest/threadHelper.inline.hpp line 148:
>> 
>>> 146: };
>>> 147: 
>>> 148: // A TestThreadGroup tracks multiple threads running the same function.
>> 
>> So we have F, which is kind of clear, S, which is a state?, and then also a state generator? Which apparently needs to define operator () and return a state? Could you please document what the template parameters are and what is expected of them?
>> 
>> Does N really need to be a template parameter? You lose flexibility, and it is less readable too.
>
>>So we have F, which is kind of clear, S, which is a state?, and then also a state generator? Which apparently needs to define operator () and return a state?
> 
> Let me explain my reasoning, call me out if something seems odd please!
> 
> A `BasicTestThread` has a F, the code which runs, and S, the local state which is only available for F. A ´TestThreadGroup` let's you create many `BasicTestThread` for the same F, but with varying S, with the help of a state generator.
> 
> Subclasses (the "old" way) are good at having their own local state, whilst lambdas are good at sharing state (through their access to the lexical environment they're created in). The whole S and state generator business is kind of a crutch to help lambdas out to be more like subclasses. Maybe there's a bit of YAGNI here, and really it's fine if `TestThreadGroup` also gets an S like `BasicTestThread` does?
> 
>>Could you please document what the template parameters are and what is expected of them?
> 
> Absolutely. I'll also lengthen their name, to make them more clear-
> 
>>Does N really need to be a template parameter? You lose flexibility, and it is less readable too.
> 
> What flexibility are we losing? We're not going to be dynamically reallocate the amount of threads in a `TestThreadGroup`, or is this the exact use case you're imagining in the future?
> 
> I don't think it's necessarily less readable. It's fairly standard in C++ STL to use template arguments for sizes (see `std::array`). I know we typically use raw pointers and length variables in Hotspot, so it's certainly unidiomatic from those eyes. To me, it's more readable, because it's a hint to me that "aha, this size won't change during execution".

Hi @jdksjolen,

> > So we have F, which is kind of clear, S, which is a state?, and then also a state generator? Which apparently needs to define operator () and return a state?
> 
> Let me explain my reasoning, call me out if something seems odd please!
> 
> A `BasicTestThread` has a F, the code which runs, and S, the local state which is only available for F. A ´TestThreadGroup`let's you create many`BasicTestThread` for the same F, but with varying S, with the help of a state generator.

Okay, I see. Thanks for clarifying. All examples atm hand out the same state to the individual threads though. Do we have many examples where we have different states?

One simpler, more pragmatic way would be for the thread group to give each started thread a numeric id. Most of the cases I can think of would be happy with that, using the id inside the thread function to control behavior.

> 
> Subclasses (the "old" way) are good at having their own local state, whilst lambdas are good at sharing state (through their access to the lexical environment they're created in). The whole S and state generator business is kind of a crutch to help lambdas out to be more like subclasses. Maybe there's a bit of YAGNI here, and really it's fine if `TestThreadGroup` also gets an S like `BasicTestThread` does?

Well, I don't see why this would have to be templatized at all; normal inheritance and interfaces would work just as well. At least with the examples given, I don't see the performance advantage. That's not to say that I'm against it, the in-function lambda syntax is certainly nice.

> 
> > Could you please document what the template parameters are and what is expected of them?
> 
> Absolutely. I'll also lengthen their name, to make them more clear-
> 
> > Does N really need to be a template parameter? You lose flexibility, and it is less readable too.
> 
> What flexibility are we losing? We're not going to be dynamically reallocate the amount of threads in a `TestThreadGroup`, or is this the exact use case you're imagining in the future?

The ability to start with the number of threads dictated at runtime, depending on runtime conditions. The ability to reuse the same TTG in different subtests with different thread numbers without template bloat.

Actually, the same argument goes for F. This design still requires us to use the same thread function for all threads. For cases where you want to have multiple different thread functions, you would still end up with some sort of switch inside the thread function. If F were a runtime thing, you could do something like this:


TTG.add_threads(12, functorX); // add 12 threads that do X
TTG.add_threads(6, functorY); // add 12 threads that do Y


i.e., add multiple threads with multiple functions to the same thread group and control them all via TTG.

> 
> I don't think it's necessarily less readable. It's fairly standard in C++ STL to use template arguments for sizes (see `std::array`). I know we typically use raw pointers and length variables in Hotspot, so it's certainly unidiomatic from those eyes. To me, it's more readable, because it's a hint to me that "aha, this size won't change during execution".

As I wrote in the other comment, opinions vary on this, and we have many devs who need to maintain the code base.

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

PR: https://git.openjdk.org/jdk/pull/9962