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

[Thomas Stüfe]

Hi Mikael,

On Mon, Jul 17, 2017 at 4:41 PM, Mikael Gerdin <mikael.gerdin at oracle.com>
wrote:

> Hi Thomas,
>
> On 2017-07-17 16:33, Thomas Stüfe wrote:
>
>> Hi Mikael,
>>
>> thanks for your review!
>>
>> New Webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8179327-gtestLaun
>> cher-should-have-an-option-to-avoid-primordial-thread-for-
>> tests/webrev.01/webrev/
>>
>> See comments inline.
>>
>
> Thanks for fixing this, it looks a lot nicer now.
> I can sponsor the change for you. I think Robbin is on vacation but I
> don't think he'll mind a bit of additional cleanups.
>
> /Mikael
>
>
Thanks! That would be nice!

..Thomas


>
>> On Mon, Jul 17, 2017 at 10:56 AM, Mikael Gerdin <mikael.gerdin at oracle.com
>> <mailto: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 <mailto:thomas.stuefe at gmail.com>>
>>         wrote:
>>
>>
>>
>>             On Tue, Jul 4, 2017 at 10:22 AM, Robbin Ehn
>>             <robbin.ehn at oracle.com <mailto: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
>>                     <https://bugs.openjdk.java.net/browse/JDK-8179327>>
>>                     Webrev: http://cr.openjdk.java.net/~stuefe/webrevs/
>>                     <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
>>                     <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
>>
>>
>>
>>
>>