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

[Evgeny Nikitin]

Hi David,

 >  > 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.

I finally decided to leave it as it is now, since printf doesn't add a 
newline and System.lineSeparator() is not shorter than String.format() 
:). Please let me know if you disagree.

Best regards,
Evgeny.


On 2020-04-07 16:09, Evgeny Nikitin wrote:
> 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.
>>>>>