RFR(s): 8179327: gtestLauncher should run tests on a separate thread (optionally)

[Thomas Stüfe]

Hi Mikael,

thanks for your review!

New Webrev:
http://cr.openjdk.java.net/~stuefe/webrevs/8179327-gtestLauncher-should-have-an-option-to-avoid-primordial-thread-for-tests/webrev.01/webrev/

See comments inline.

On Mon, Jul 17, 2017 at 10:56 AM, Mikael Gerdin <mikael.gerdin at oracle.com>
wrote:

> Hi Thomas,
>
> On 2017-07-13 08:18, Thomas Stüfe wrote:
>
>> Ping..
>>
>> May I please get a second review and a sponsor?
>>
>
> I would prefer if you made a
> const static bool DEFAULT_SPAWN_IN_NEW_THREAD which is initialized with
> the proper platform value. The current 5-line return statement in
> get_spawn_new_main_thread_arg looks a bit weird.
>

Good suggestion, fixed.


>
> If you move the wrapping of argc/argv up to runUnitTests and pass the
> args_t instead you can make run_in_new_thread completely platform specific
> instead of having #ifdef _WIN32 in the middle, I think this is the
> preferred style in the code base overall.
>
>
Okay. I did this and shuffled a bit code around, so now we only have one
#ifdef _WIN32 section, should be better readable now.


> In the pthread join error path you have
> "/rsyn" at the end of the line, is that intentional?
>

No, good catch, that was a stray console command.

Kind Regards, Thomas


>
> Thanks
> /Mikael
>
>
>
>> Thanks, Thomas
>>
>> On Tue, Jul 4, 2017 at 11:12 AM, Thomas Stüfe <thomas.stuefe at gmail.com>
>> wrote:
>>
>>
>>>
>>> On Tue, Jul 4, 2017 at 10:22 AM, Robbin Ehn <robbin.ehn at oracle.com>
>>> wrote:
>>>
>>> Looks reasonable, thanks!
>>>>
>>>>
>>>> Thank you!
>>>
>>>
>>> /Robbin
>>>>
>>>> If you don't mind, please don't wrap lines, links do not work well with
>>>> line breaks in them :)
>>>>
>>>>
>>>> Oops, sorry!
>>>
>>>
>>> On 07/02/2017 09:23 AM, Thomas Stüfe wrote:
>>>>
>>>> Dear all,
>>>>>
>>>>> please review the following change:
>>>>>
>>>>> Bug: JDK-8179327 <https://bugs.openjdk.java.net/browse/JDK-8179327>
>>>>> Webrev: http://cr.openjdk.java.net/~stuefe/webrevs/
>>>>> 8179327-gtestLauncher-should-have-an-option-to-avoid-
>>>>> primordial-thread-for-tests/webrev.00/webrev/
>>>>>
>>>>> This fix is motivated by the inability of the AIX openjdk vm to run on
>>>>> the
>>>>> primordial thread (see https://bugs.openjdk.java.net/
>>>>> browse/JDK-8171505
>>>>> ),
>>>>> which we unfortunately cannot do much about. This prevented us until
>>>>> now
>>>>> to
>>>>> run gtests successfully, because the gtestlauncher (in contrast to the
>>>>> standard java launcher) invokes the VM on the primordial thread.
>>>>>
>>>>> To fix this, I gave the gtest launcher the ability to run the tests in
>>>>> an
>>>>> own thread. Instead of sprinkling the source with "#ifdef _AIX", I
>>>>> added
>>>>> a
>>>>> command line option "-new-thread", which allows to start the tests in a
>>>>> new
>>>>> thread, and implemented this feature for all platforms (well, posix and
>>>>> windows). On all platforms save AIX this is by default off, so nothing
>>>>> changes. On AIX, tests will be default be executed in a new thread.
>>>>>
>>>>> Thanks for reviewing,
>>>>>
>>>>> Kind Regards, Thomas
>>>>>
>>>>>
>>>>>
>>>