RFR(XS): 8174768: Make ProcessTools print executed process output into a separate file

[Evgeny Nikitin]

Hi David,

 > why did you introduce a new block scope?

To separate the action block from the other code. "A Poor/lazy man's 
method". I've preferred to lay it out this way because it makes the code 
more compact and easy to read (as opposing to looking for a only-once 
used method in some other part of the file) and due to the confusing 
name of OutputAnalyzer type (which I need as a storage for output, not 
as some analyzer). Creating a method with OutputAnalyzer as a parameter 
would make this method's purpose even more confusing.

 > You can use printf rather than making a separate call to format.
Good point, I've missed that method. I'll fix it and ask Igor to change 
the webrev.

Regards,

Evgeny.

On 2020-04-07 14:50, David Holmes wrote:
> Hi Evgeny,
>
> On 7/04/2020 6:33 pm, Evgeny Nikitin wrote:
>> Hi David,
>>
>>> I'm not at all sure this is generally what we want for every single 
>>> test that uses ProcessTools! But I'm willing it to see it trialed. 
>>
>> Well, it's mostly used for test VM runs. In runs I performed 
>> (artificial "failures" included) the amounts of output were very small.
>>
>>> Please run full tier testing at least to tier 6 and ideally beyond 
>>> before pushing this. There are potential implications for temporary 
>>> (and more permanent) disk usage as well as additional time needed to 
>>> write files out to disk. (Hopefully these are generally small enough 
>>> that this doesn't make a noticeable difference.)
>> Done, thanks for suggestion. The additional runs showed no problems.
>
> Good to know - thanks.
>
>> I've provided Igor with a slightly modified version (Added 
>> notification about the output file into the main test's log). Please 
>> review:
>>
>> http://cr.openjdk.java.net/~iignatyev/enikitin/8174768/webrev.01/
>
> A couple of minor nits:
>
>  397             {
>
> why did you introduce a new block scope?
>
>  404                 System.out.println(String.format(
>
> You can use printf rather than making a separate call to format.
>
> No need to see any update if you chose to make it.
>
> Thanks,
> David
> -----
>
>> Best Regards,
>>
>> Evgeny.
>>
>> On 2020-04-02 02:07, David Holmes wrote:
>>> Thanks for sharing this Igor!
>>>
>>> I'm not at all sure this is generally what we want for every single 
>>> test that uses ProcessTools! But I'm willing it to see it trialed.
>>>
>>> Evgeny: Please run full tier testing at least to tier 6 and ideally 
>>> beyond before pushing this. There are potential implications for 
>>> temporary (and more permanent) disk usage as well as additional time 
>>> needed to write files out to disk. (Hopefully these are generally 
>>> small enough that this doesn't make a noticeable difference.)
>>>
>>> Thanks,
>>> David
>>>
>>> On 2/04/2020 5:13 am, Igor Ignatyev wrote:
>>>> Hi Evgeny,
>>>>
>>>> (widening the audience, given this affects not just hotspot 
>>>> compiler, but hotspot tests as well as core libs tests in general)
>>>>
>>>> overall that looks good to me. one suggestion, for the ease of 
>>>> failure analysis it might be worth to print out the names of 
>>>> created files, although this might potentially clutter the output, 
>>>> I don't think it'll be a problem given we already print out things 
>>>> like 'Gathering output for process ...' , 'Waiting for 
>>>> completion...' in LazyOutputBuffer.
>>>>
>>>>> The change has been tested via a mach5 test runs (jdk-tier1 
>>>>> through 4) on the 4 common platforms (linux-x64, windows-x64, 
>>>>> macosx-x64, sparcv9).
>>>> this doesn't include any of hotspot tiers, could you please also 
>>>> run hs-tier1--4?
>>>> // you can use tierN jobs which include both jdk and hs parts.
>>>>
>>>> Thanks,
>>>> -- Igor
>>>>
>>>>> On Mar 30, 2020, at 3:55 AM, Evgeny Nikitin 
>>>>> <evgeny.nikitin at oracle.com> wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8174768
>>>>>
>>>>> Webrev: 
>>>>> http://cr.openjdk.java.net/~iignatyev/enikitin/8174768/webrev.00/
>>>>>
>>>>>
>>>>> The bug had been created as a request to simplify investigation 
>>>>> for compiler control tests failures.
>>>>> I found the functionality pretty generic and useful and made 
>>>>> ProcessTools dump output as well as some diagnostic information 
>>>>> for every executed process into a separate file.
>>>>> The diagnostic information contains cmdline, exit code, stdout and 
>>>>> stderr. The output files are named like 'pid-<PID>-output.log'.
>>>>>
>>>>> The change has been tested via a mach5 test runs (jdk-tier1 
>>>>> through 4) on the 4 common platforms (linux-x64, windows-x64, 
>>>>> macosx-x64, sparcv9).
>>>>>
>>>>> Please review,
>>>>> /Evgeny Nikitin.
>>>>