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