[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR 8196324: Update FilterMatch & FilterNoMatch to use TestScaffold instead of JDIScaffold
From: Paru Somashekar <parvathi.somashekar () oracle ! com>
Date: 2018-02-15 3:01:12
Message-ID: 5A84F7F8.9010500 () oracle ! com
[Download RAW message or body]
Sound great, thanks Serguei.
thanks,
Paru.
On 2/14/18, 4:50 PM, serguei.spitsyn@oracle.com wrote:
> Paru,
>
> Thank you for the update.
>
> Just noticed one more thing in both files - extra space before the
> parameter:
> + addListener (this);
>
> Otherwise, it looks good.
> No need in another webrev.
>
> Thanks,
> Serguei
>
>
>
> On 2/14/18 16:39, Paru Somashekar wrote:
>> Thanks Serguei, made the changes you have suggested,
>>
>> and the latest review is at:
>> http://cr.openjdk.java.net/~psomashe/8196324/webrev.02/
>> <http://cr.openjdk.java.net/%7Epsomashe/8196324/webrev.02/>
>>
>> thanks,
>> Paru.
>>
>>
>> On 2/14/18, 2:30 PM, serguei.spitsyn@oracle.com
>> <mailto:serguei.spitsyn@oracle.com> wrote:
>>> Hi Paru,
>>>
>>> Thank you for the update!
>>>
>>> Could you, please, rearrange a couple of more places in both files?
>>>
>>> 66 BreakpointEvent bpe = startToMain("HelloWorld");
>>> 67 ReferenceType referenceType = (ClassType)bpe.location().declaringType();
>>> 68
>>> 69 mainThread = bpe.thread();
>>> 70 // VM has started, but hasn't started running the test program yet.
>>> 71 EventRequestManager requestManager = vm().eventRequestManager();
>>> 72
>>> 73 Location loc = findLocation(referenceType, 3);
>>> 74
>>> 75 BreakpointRequest bpRequest = requestManager.createBreakpointRequest(loc);
>>>
>>> I'd suggest to move the lines 68,69 after the line 75.
>>> Also, the empty lines 72, 74 are not needed.
>>> I'm not sure, the line 70 with the comment is placed correctly or
>>> needed at all.
>>>
>>> This line needs an update of "request1":
>>> 107 //request1.addClassFilter("x");
>>>
>>> Extra space before '!' and missed space before '{' :
>>> 115 if ( !stepCompleted){
>>>
>>>
>>> Let's simplify/unify the tracing lines below further:
>>>
>>> 130 System.out.println("Agent: StepEvent: line#=" + event.location().lineNumber()
>>> 131 + " event=" + event);
>>> . . .
>>> 141 System.out.println("Agent: BreakpointEvent " +
>>> 142 " at " + locStr + " in thread: " + thread);
>>>
>>> Something like this would be better:
>>> 130 System.out.println("StepEvent at " + event.location());
>>> . . .
>>> 141 System.out.println("BreakpointEvent at " + event.location());
>>>
>>>
>>> The lines 139 and 140 can be removed:
>>> 139 ThreadReference thread = event.thread();
>>> 140 String locStr = "" + event.location();
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>>
>>> On 2/13/18 23:01, Paru Somashekar wrote:
>>>> Hi Serguei,
>>>>
>>>> Thanks very much for the review and I have made all the changes
>>>> incorporating your feedback.
>>>>
>>>> New Webrev at
>>>> :http://cr.openjdk.java.net/~psomashe/8196324/webrev.01/
>>>> <http://cr.openjdk.java.net/%7Epsomashe/8196324/webrev.01/>
>>>>
>>>> thanks,
>>>> Paru.
>>>>
>>>> On 2/13/18, 2:02 PM, serguei.spitsyn@oracle.com
>>>> <mailto:serguei.spitsyn@oracle.com> wrote:
>>>>> Hi Paru,
>>>>>
>>>>> It looks good in general but I'd like to ask for some cleanup.
>>>>>
>>>>> I tell just about the first test (FilterMatch) but the other one
>>>>> needs the same.
>>>>>
>>>>> 132 // This gets called if all filters match.
>>>>> 133 public void stepCompleted(StepEvent event) {
>>>>> 134 listenCalled = true;
>>>>> 135 System.out.println("listen: line#=" + event.location().lineNumber()
>>>>> 136 + " event=" + event);
>>>>> 137 // disable the step and then run to completion
>>>>> 138 StepRequest str= (StepRequest)event.request();
>>>>> 139 str.disable();
>>>>> 140 eventSet.resume();
>>>>> 141 }
>>>>> I'd suggest to replace "listen:" above with "Agent: StepEvent:"
>>>>> to make
>>>>> it more consistent with similar tracing in the
>>>>> breakpointReached() below.
>>>>>
>>>>> 143 public void breakpointReached(BreakpointEvent event) {
>>>>> 144 ThreadReference thread = ((BreakpointEvent) event).thread();
>>>>> 145 String locStr = "" + ((BreakpointEvent) event).location();
>>>>> 146 System.out.println("Agent: BreakpointEvent #" +
>>>>> 147 " at " + locStr + " in thread: " + thread);
>>>>> 148 // The bkpt was hit; disable it.
>>>>> 149 request.disable();
>>>>> 150 }
>>>>> The casts to BreakpointEvent at lines 144, 145 are not needed
>>>>> as the "event" is already of this type.
>>>>> Unneeded sign '#' at the line 146.
>>>>> Also, I'm suggesting to disable the "request" the same way
>>>>> as it is done in the stepCompleted():
>>>>> BreakpointRequest bpr= (BreakpointRequest)event.request();
>>>>> bpr.disable();
>>>>>
>>>>> It will help to make the "request" local in the runTests().
>>>>> Unneeded empty lines: 52, 54:
>>>>> 51 EventSet eventSet = null;
>>>>> 52
>>>>> 53 static boolean listenCalled;
>>>>> 54
>>>>> 55 BreakpointRequest request;
>>>>>
>>>>> The listenCalled needs to be renamed to stepCompleted.
>>>>> There is no big need for it to be static as it is similar to the
>>>>> eventSet.
>>>>> It is better to initialize it with false.
>>>>> Then this line can be removed:
>>>>> 114 listenCalled = false;
>>>>>
>>>>> I'd suggest to rename the variables "request" and "request1"
>>>>> to "bpRequest" and "stepRequest" and make the 'bpRequest' to be local
>>>>> in the runTests() as it is the only place where it has to be used.
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>> On 2/12/18 11:25, Paru Somashekar wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please review fix for JDK-8196324
>>>>>>
>>>>>> The includes the following changes:
>>>>>>
>>>>>> * Use startToMain method of TestScaffold that automatically calls
>>>>>> connect() and waitForVMStart()
>>>>>> * Since TestScaffold extends TargetAdapter, it now overrides
>>>>>> event callbacks directly in the test class rather than implement
>>>>>> an anonymous inner class.
>>>>>> * Breakpoint handling is done in breakpointReached callback
>>>>>> instead of waitForRequestedEvent and subsequently removing it.
>>>>>>
>>>>>> Bug : https://bugs.openjdk.java.net/browse/JDK-8196324
>>>>>> Review : http://cr.openjdk.java.net/~psomashe/8196324/webrev/
>>>>>> <http://cr.openjdk.java.net/%7Epsomashe/8196324/webrev/>
>>>>>> <http://cr.openjdk.java.net/%7Epsomashe/8196324/webrev/>
>>>>>>
>>>>>> thanks,
>>>>>> Paru.
>>>>>
>>>>
>>>
>>
>
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Sound great, thanks Serguei.<br>
<br>
thanks,<br>
Paru.<br>
<br>
On 2/14/18, 4:50 PM, <a class="moz-txt-link-abbreviated" \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote: \
<blockquote cite="mid:bf139543-6398-f04d-b931-e7592cdb29d6@oracle.com"
type="cite">
<meta http-equiv="Context-Type" content="text/html; charset=utf-8">
<div>Paru,<br>
<br>
Thank you for the update.<br>
<br>
Just noticed one more thing in both files - extra space before
the parameter:<br>
<pre><span>+ addListener (this);</span></pre>
<br>
Otherwise, it looks good.<br>
No need in another webrev.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
<br>
On 2/14/18 16:39, Paru Somashekar wrote:<br>
</div>
<blockquote type="cite" cite="mid:5A84D6D1.1020400@oracle.com">
Thanks Serguei, made the changes you have suggested,<br>
<br>
and the latest review is at:<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Epsomashe/8196324/webrev.02/">http://cr.openjdk.java.net/~psomashe/8196324/webrev.02/</a><br>
<br>
thanks,<br>
Paru.<br>
<br>
<br>
On 2/14/18, 2:30 PM, <a moz-do-not-send="true"
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
wrote:
<blockquote
cite="mid:e16ecdec-9716-77c2-5566-89fb8a270fa1@oracle.com"
type="cite">
<div>Hi Paru,<br>
<br>
Thank you for the update!<br>
<br>
Could you, please, rearrange a couple of more places in both
files?<br>
<br>
<pre><span> 66 BreakpointEvent bpe = \
startToMain("HelloWorld");</span> <span> 67 ReferenceType referenceType = \
(ClassType)bpe.location().declaringType();</span> 68
<span> 69 mainThread = bpe.thread();</span>
70 // VM has started, but hasn't started running the test program yet.
71 EventRequestManager requestManager = vm().eventRequestManager();
72
<span> 73 Location loc = findLocation(referenceType, 3);</span>
<span> 74 </span>
<span> 75 BreakpointRequest bpRequest = \
requestManager.createBreakpointRequest(loc);</span></pre> <br>
I'd suggest to move the lines 68,69 after the line 75. <br>
Also, the empty lines 72, 74 are not needed.<br>
I'm not sure, the line 70 with the comment is placed
correctly or needed at all.<br>
<br>
This line needs an update of "request1":<br>
<pre> 107 //request1.addClassFilter("x");</pre>
<br>
Extra space before '!' and missed space before '{' :<br>
<pre><span> 115 if ( !stepCompleted){
Let's simplify/unify the tracing lines below further:
</span>
<span><span> 130 System.out.println("Agent: StepEvent: line#=" + \
event.location().lineNumber()</span> <span> 131 + " event=" + \
event);</span> </span> . . .
<span> 141 System.out.println("Agent: BreakpointEvent " +</span>
<span> 142 " at " + locStr + " in thread: " + thread);</span>
Something like this would be better:
<span><span> 130 System.out.println("StepEvent at " + \
event.location()</span><span>);</span> </span> . . .
<span> 141 System.out.println("BreakpointEvent at " +</span> \
<span><span>event.location()</span><span>);</span></span>
The lines 139 and 140 can be removed:
<span><span> 139 ThreadReference thread = event.thread();
</span> 140 String locStr = "" + event.location();</span>
<span></span></pre>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
<br>
On 2/13/18 23:01, Paru Somashekar wrote:<br>
</div>
<blockquote type="cite" cite="mid:5A83DEBE.1010709@oracle.com">
Hi Serguei,<br>
<br>
Thanks very much for the review and I have made all the
changes incorporating your feedback.<br>
<br>
New Webrev at :<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Epsomashe/8196324/webrev.01/">
http://cr.openjdk.java.net/~psomashe/8196324/webrev.01/</a><br>
<br>
thanks,<br>
Paru.<br>
<br>
On 2/13/18, 2:02 PM, <a moz-do-not-send="true"
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
wrote:
<blockquote
cite="mid:c79810ca-52f3-ecbf-c02f-87a728f1877f@oracle.com"
type="cite">
<div>Hi Paru,<br>
<br>
It looks good in general but I'd like to ask for some
cleanup.<br>
<br>
I tell just about the first test (FilterMatch) but the
other one needs the same.<br>
<br>
<pre><span> 132 // This gets called if all filters match.</span>
<span> 133 public void stepCompleted(StepEvent event) {</span>
<span> 134 listenCalled = true;</span>
<span> 135 System.out.println("listen: line#=" + \
event.location().lineNumber()</span> <span> 136 + " event=" + \
event);</span> <span> 137 // disable the step and then run to \
completion</span> <span> 138 StepRequest str= \
(StepRequest)event.request();</span> <span> 139 str.disable();</span>
<span> 140 eventSet.resume();</span>
<span> 141 }</span></pre>
I'd suggest to replace "listen:" above with "Agent:
StepEvent:" to make<br>
it more consistent with similar tracing in the
breakpointReached() below.<br>
<br>
<pre><span> 143 public void breakpointReached(BreakpointEvent \
event) {</span> <span> 144 ThreadReference thread = ((BreakpointEvent) \
event).thread();</span> <span> 145 String locStr = "" + ((BreakpointEvent) \
event).location();</span> <span> 146 System.out.println("Agent: \
BreakpointEvent #" +</span> <span> 147 " at " + locStr + " in thread: \
" + thread);</span> <span> 148 // The bkpt was hit; disable it.</span>
<span> 149 request.disable();</span>
<span> 150 }</span></pre>
The casts to BreakpointEvent at lines 144, 145 are not
needed<br>
as the "event" is already of this type.<br>
Unneeded sign '#' at the line 146.<br>
Also, I'm suggesting to disable the "request" the same
way<br>
as it is done in the stepCompleted():<br>
<pre><span> BreakpointRequest bpr= \
(</span><span><span>BreakpointRequest</span>)event.request();</span> <span> \
bpr.disable();
It will help to make the "request" local in the runTests().
</span></pre>
Unneeded empty lines: 52, 54:<br>
<pre><span> 51 EventSet eventSet = null;</span>
52
53 static boolean listenCalled;
54
<span> 55 BreakpointRequest request;</span>
</pre>
<br>
The listenCalled needs to be renamed to stepCompleted.<br>
There is no big need for it to be static as it is
similar to the eventSet.<br>
It is better to initialize it with false.<br>
Then this line can be removed:<br>
<pre><span> 114 listenCalled = false;</span></pre>
<br>
I'd suggest to rename the variables "request" and
"request1"<br>
to "bpRequest" and "stepRequest" and make the
'bpRequest' to be local<br>
in the runTests() as it is the only place where it has
to be used.<br>
<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 2/12/18 11:25, Paru Somashekar wrote:<br>
</div>
<blockquote type="cite"
cite="mid:5A81EA18.9080808@oracle.com">Hi, <br>
<br>
Please review fix for JDK-8196324 <br>
<br>
The includes the following changes: <br>
<br>
* Use startToMain method of TestScaffold that
automatically calls connect() and waitForVMStart() <br>
* Since TestScaffold extends TargetAdapter, it now
overrides event callbacks directly in the test class
rather than implement an anonymous inner class. <br>
* Breakpoint handling is done in breakpointReached
callback instead of waitForRequestedEvent and
subsequently removing it. <br>
<br>
Bug : <a moz-do-not-send="true"
href="https://bugs.openjdk.java.net/browse/JDK-8196324">https://bugs.openjdk.java.net/browse/JDK-8196324</a>
<br>
Review : <a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Epsomashe/8196324/webrev/">http://cr.openjdk.java.net/~psomashe/8196324/webrev/</a>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Epsomashe/8196324/webrev/"><http://cr.openjdk.java.net/%7Epsomashe/8196324/webrev/></a>
<br>
<br>
thanks, <br>
Paru. <br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic