Re: RFR: 8059625 - JEP-JDK-8043304: Test task: DTrace- tests for segmented codecache feature

[serguei.spit...@oracle.com]

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