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