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

[serguei.spitsyn at 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
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20141223/b86f40b8/attachment.html>