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

[David Holmes]

Looks good.

Thanks,
David

On 9/04/2020 3:27 am, Evgeny Nikitin wrote:
> Thanks to Igor Ignatyev, a new version is available at
> 
> http://cr.openjdk.java.net/~iignatyev/enikitin/8174768/webrev.02
> 
> Regards,
> Evgeny.
> 
> 
> On 2020-04-08 17:19, Igor Ignatev wrote:
>> You can use %n in printf to get a system-specific newline.
>>
>> — Igor
>>
>>> On Apr 8, 2020, at 8:12 AM, Evgeny Nikitin 
>>> <evgeny.nikitin at oracle.com> wrote:
>>>
>>> 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.
>>>>>>>>
>>