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

[serguei.spitsyn at oracle.com]

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
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20141223/257ac591/attachment-0001.html>