Hi Dmitrij,
DtraceRunner::runDtrace prints warning in case of non-zero exit code.
It means that the test will pass even if traced JVM crashed.
I can name several issues that were found accidentally, because tests
didn't care about a crash.
Are there any other sane reasons for DTrace to return non-zero exit code
except lack of privileges?
Igor's suggestion will prevent us from running DTrace without required
privileges, so I'd prefer to see the test failure instead of a warning in case
of non-zero exit code returned by DTrace.
Also, please add @bug to the test.
Thanks,
Filipp.
On 12/25/2014 02:05 PM, Igor Ignatyev wrote:
I'd prefer to check privileges by running dtrace against dtrace;
and do it in DtraceRunner::dtraceAvailable, smth like
result = false;
dtrace = getDtracePath();
if ($dtrace) {
$dtrace $dtrace
result = $? == 0;
}
return $result;
Igor
On 12/24/2014 11:09 PM, Dmitrij Pochepko wrote:
Hi,
i've placed a guard which skips test in case dtrace exits with non-zero
code.
http://cr.openjdk.java.net/~iignatyev/dpochepk/8059625/webrev.02/
Thanks,
Dmitrij
Hi,
There is one more issues related to these tests(not directly).
Currently, dtrace tests can't be run in jprt, because dtrace require
additional previleges for launching user.
Perhaps it's just user to be granted previleges in jprt solaris hosts...
Not sure about some guard in tests... the only way i know so far is to
just try launching dtrace and parse output
saying about missing previleges, but it doesn't seem to be good solution.
I think test should assume to be launched in correct environment and
in case it's failed because of previleges, evaluating engineer
starting to work on respective env. issue...
Does anybody have some additional concerns?
Thanks,
Dmitrij
Hi Dmitrij,
There is still a typo in spelling (DESCTRUCTIVE => DESTRUCTIVE):
test/testlibrary/com/oracle/java/testlibrary/dtrace/DtraceRunner.java
42 public static final String
PERMIT_DESCTRUCTIVE_ACTIONS_DTRACE_OPTION = "w";
test/compiler/codecache/dtrace/SegmentedCodeCacheDtraceTest.java
85 DtraceRunner.PERMIT_DESCTRUCTIVE_ACTIONS_DTRACE_OPTION,
Otherwise, looks good.
Please, consider it reviewed (no need to re-review).
Thanks,
Serguei
On 12/23/14 3:36 AM, Dmitrij Pochepko wrote:
Hi,
please review updated webrev
http://cr.openjdk.java.net/~iignatyev/dpochepk/8059625/webrev.01/
Thanks,
Dmitrij
On 12/23/14 12:37 AM, Dmitrij Pochepko wrote:
Hi,
Thank you for catching these issues.
I have a question regarding last comment: does it make any
difference to change "reading of static member 3 times" to
"copying into static member of another class and then read it 3
times"?
Yes, it does (it is a minor issue though).
It is because the class names are pretty big.
It would simplify the code and improve readability, right?
You already do it two times:
179 List<Executable> tml
180 =
SegmentedCodeCacheDtraceTestWorker.TESTED_METHODS_LIST;
...
235 List<Executable> mlist
236 =
SegmentedCodeCacheDtraceTestWorker.TESTED_METHODS_LIST;
My suggestion is to do it once somewhere before the line 74:
static final List<Executable> MLIST =
SegmentedCodeCacheDtraceTestWorker.TESTED_METHODS_LIST;
and then use the mlist in other places where it is needed:
75 String params = MLIST.stream()
...
182 d.put(MLIST.get(i).getName(), new
MethodData(MLIST.get(i).getName(),
...
239 for (int i = MLIST.size() - 1; i > -1; i--) {
240 sb.append(MLIST.get(i).getName()).append(delimeter);
These lines will go away:
179 List<Executable> tml
180 =
SegmentedCodeCacheDtraceTestWorker.TESTED_METHODS_LIST;
...
235 List<Executable> mlist
236 =
SegmentedCodeCacheDtraceTestWorker.TESTED_METHODS_LIST;
Both private static classes are inner classes of the public one.
The MLIST will be in a context for the inner classes, and so, needs
no prefixing.
Thanks,
Serguei
Thanks,
Dmitrij
Hi Dmitry,
It looks good in general.
Some minor comments are below.
test/testlibrary/com/oracle/java/testlibrary/dtrace/DtraceRunner.java
42 public static final String
PERMIT_DESCTUCTIVE_ACTIONS_DTRACE_OPTION = "w";
A typo in the constant name: DESCTUCTIVE => DESTRUCTIVE
test/compiler/codecache/dtrace/SegmentedCodeCacheDtraceTest.java
84 DtraceRunner.PERMIT_DESCTUCTIVE_ACTIONS_DTRACE_OPTION,
A typo in the constant name: DESCTUCTIVE => DESTRUCTIVE
60 private static final String WORKER_CLASS_NAME
61 = SegmentedCodeCacheDtraceTestWorker.class.getName();
...
80 runner.runDtrace(JDKToolFinder.getTestJDKTool("java"), JAVA_OPTS,
81 SegmentedCodeCacheDtraceTestWorker.class.getName(), params,
The WORKER_CLASS_NAME can be used at 81.
75 String params =
SegmentedCodeCacheDtraceTestWorker.TESTED_METHODS_LIST.stream()
...
179 List<Executable> tml
180 =
SegmentedCodeCacheDtraceTestWorker.TESTED_METHODS_LIST;
...
235 List<Executable> mlist
236 =
SegmentedCodeCacheDtraceTestWorker.TESTED_METHODS_LIST;
The TESTED_METHODS_LIST is used three times.
It can be cached at the top and re-used.
Thanks,
Serguei
On 12/19/14 11:03 AM, Dmitrij Pochepko wrote:
Hi all,
Please review changes for
https://bugs.openjdk.java.net/browse/JDK-8059625 -
JEP-JDK-8043304: Test task: DTrace- tests for segmented
codecache feature
Description: this fix introduce dtrace test, which verify that
different combinations of available compile levels(and, in case
compile levels allows it, different code heaps as result)
doesn't affect callstack shown by dtrace. There is a control
class SegmentedCodeCacheDtraceTest.java and class for running
via dtrace SegmentedCodeCacheDtraceTestWorker.java. A dtrace d
script is also present (SegmentedCodeCacheDtraceTestScript.d). A
control class is using DtraceRunner.java to run dtrace and then
analyzing results using class
SegmentedCodeCacheDtraceResultsAnalyzer with
DtraceResultsAnalyzer interface.
There is also a small class CompilerUtils.java created for
usefull common code.
webrev:
http://cr.openjdk.java.net/~iignatyev/dpochepk/8059625/webrev.00/
Additional note: Please note that this path assumes that fix for
JDK-8066440 - Various changes in testlibrary for JDK-8059613 is
also applied.
Thanks,
Dmitrij
