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