RFR 8204695: [Graal] vmTestbase/nsk/jdi/ClassPrepareRequest/addSourceNameFilter/addSourceNameFilter002/addSourceNameFilter002.java fails
Daniil Titov
daniil.x.titov at oracle.com
Thu Jul 19 18:06:47 UTC 2018
Thank you Chris and Serguei for reviewing this change!
Best regards,
Daniil
From: Chris Plummer <chris.plummer at oracle.com>
Date: Thursday, July 19, 2018 at 11:03 AM
To: Daniil Titov <daniil.x.titov at oracle.com>, "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>, "serviceability-dev at openjdk.java.net serviceability-dev at openjdk.java.net" <serviceability-dev at openjdk.java.net>
Subject: Re: RFR 8204695: [Graal] vmTestbase/nsk/jdi/ClassPrepareRequest/addSourceNameFilter/addSourceNameFilter002/addSourceNameFilter002.java fails
Ok, your changes look good then.
thanks,
Chris
On 7/19/18 11:01 AM, Daniil Titov wrote:
Hi Chris,
Some events are still coming in after disable() returns. The event handler sees the request object associated with this event ( event.request() ) as disabled but it still receives them.
Best regards,
Daniil
From: Chris Plummer <chris.plummer at oracle.com>
Date: Thursday, July 19, 2018 at 10:54 AM
To: Daniil Titov <daniil.x.titov at oracle.com>, "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>, "serviceability-dev at openjdk.java.net serviceability-dev at openjdk.java.net" <serviceability-dev at openjdk.java.net>
Subject: Re: RFR 8204695: [Graal] vmTestbase/nsk/jdi/ClassPrepareRequest/addSourceNameFilter/addSourceNameFilter002/addSourceNameFilter002.java fails
But the code used to be:
168 request.disable();
169
170 eventHandler.removeListener(listener);
Doesn't the disable stop any new ClassPrepareEvents from coming in, and this is done before the listener is removed, or is there a synchronization issue here, and you can still get some events coming in after disable() returns. I'm not sure if disable() makes any guarantees about the debuggee side having fully processed it and guaranteed delivery of all pending events before it returns.
thanks,
Chris
On 7/19/18 10:45 AM, Daniil Titov wrote:
Hi Chris,
It solves the problem when ClassPrepare event comes from the Graal compiler thread at the time when no any listener listening for this event is registered by the test. In this case it is handled by the EventHandler itself (lines 261-262 below) as an unexpected event and the test fails (the EventHandler has its own “default” listeners registered when the event handler starts to handle events if they are not handled by the listener the test registers).
cat -n test/hotspot/jtreg/vmTestbase/nsk/share/jdi/EventHandler.java
/**
251 * This method sets up default listeners.
252 */
253 private void createDefaultListeners() {
254 /**
255 * This listener catches up all unexpected events.
256 *
257 */
258 addListener(
259 new EventListener() {
260 public boolean eventReceived(Event event) {
261 log.complain("EventHandler> Unexpected event: " + event.getClass().getName());
262 unexpectedEventCaught = true;
263 return true;
264 }
265 }
266 );
267
Best regards,
Daniil
From: Chris Plummer <chris.plummer at oracle.com>
Date: Thursday, July 19, 2018 at 10:31 AM
To: Daniil Titov <daniil.x.titov at oracle.com>, "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>, "serviceability-dev at openjdk.java.net serviceability-dev at openjdk.java.net" <serviceability-dev at openjdk.java.net>
Subject: Re: RFR 8204695: [Graal] vmTestbase/nsk/jdi/ClassPrepareRequest/addSourceNameFilter/addSourceNameFilter002/addSourceNameFilter002.java fails
Hi Daniil,
I understand the changes in eventReceived() to filter out events that are not on the main thread, but I don't understand why you've gone from having a new listener on each iteration, to just having one listener that stays active over all iterations. What problem does that change solve?
thanks,
Chris
On 7/17/18 7:06 PM, Daniil Titov wrote:
Hi Serguei,
Please review a new version of the patch.
Webrev: http://cr.openjdk.java.net/~dtitov/8204695/webrev.03
Bug: https://bugs.openjdk.java.net/browse/JDK-8204695
Thanks!
Best regards,
Daniil
From: "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
Date: Tuesday, July 17, 2018 at 4:53 PM
To: Daniil Titov <daniil.x.titov at oracle.com>, "serviceability-dev at openjdk.java.net serviceability-dev at openjdk.java.net" <serviceability-dev at openjdk.java.net>
Subject: Re: RFR 8204695: [Graal] vmTestbase/nsk/jdi/ClassPrepareRequest/addSourceNameFilter/addSourceNameFilter002/addSourceNameFilter002.java fails
Hi Daniil,
Thank you for clarification and the webrev update!
I still have a couple of questions though.
I'd suggest more simple approach like below:
154 public boolean eventReceived(Event event) {
155 if (event instanceof ClassPrepareEvent) {
156 ClassPrepareEvent classPrepareEvent = (ClassPrepareEvent) event;
157 ThreadReference thread = classPrepareEvent.thread();
158 if (thread != null && DEBUGGEE_MAIN_THREAD.equals(thread.name())) {
159 eventReceived++;
160
161 log.display("ClassPrepareEventListener: Event received: " + event +
162 " Class: " + classPrepareEvent.referenceType().name());
163
164 vm.resume();
165
166 return true;
167 }
168 }
169
170 return false;
171 }
to something like:
public boolean eventReceived(Event event) {
if (event instanceof ClassPrepareEvent) {
ClassPrepareEvent classPrepareEvent = (ClassPrepareEvent) event;
ThreadReference thread = classPrepareEvent.thread();
if (thread != null && DEBUGGEE_MAIN_THREAD.equals(thread.name())) {
eventReceived++;
log.display("ClassPrepareEventListener: Event received: " + event +
" Class: " + classPrepareEvent.referenceType().name());
} else {
log.display("ClassPrepareEventListener: Event filtered out: " + event +
" Class: " + classPrepareEvent.referenceType().name() +
" Thread:" + classPrepareEvent.thread().name());
}
vm.resume();
return true;
}
return false;
}
245 eventHandler.startListening();
246 // Add a listener to handle ClassPrepare events fired by other (e.g. compiler) threads.
247 // The listener should be added after the event listener is started to ensure that it
248 // called before the default event listener that handles unexpected events.
249 eventHandler.addListener(new DefaultClassPrepareEventListener());
Still unclear why addListener() is invoked after startListening() but not before.
It can be that a place add this listener is not right and have to be moved into testSourceFilter().
But I hope this fragment is not needed with the simplified approach.
Otherwise, it looks good.
Thanks,
Serguei
On 7/17/18 14:55, Daniil Titov wrote:
Hi Serguei,
The test starts the event handler (nsk.share.jdi.EventHandler) and then iterates several times calling testSourceFilter() method passing there different parameters. The testSourceFilter() method does the following:
1. creates a ClassPrepareRequest object
2. registers new ClassPrepareEventListener
3. sends a command to debuggee to a load test class
4. waits till the debuggee performed the command
5. removes ClassPrepareEventListener
6. checks if a ClassPrepareEvent was received
Upon its start the EventHandler creates a default list of events listeners. The last listener in this list handles unexpected events (that are events not processed by the previous listeners)
cat -n test/hotspot/jtreg/vmTestbase/nsk/share/jdi/EventHandler.java
/**
251 * This method sets up default listeners.
252 */
253 private void createDefaultListeners() {
254 /**
255 * This listener catches up all unexpected events.
256 *
257 */
258 addListener(
259 new EventListener() {
260 public boolean eventReceived(Event event) {
261 log.complain("EventHandler> Unexpected event: " + event.getClass().getName());
262 unexpectedEventCaught = true;
263 return true;
264 }
265 }
266 );
267
On step 2 above the ClassPrepareEventListener is added at the head of the list of the listeners. It handles ClassPrepareEvents and prevents the next listeners from being invoked by returning "true" from its eventReceived(Event) method.
With Graal turned on after step 1 the JVMTI compiler thread starts sending class prepare events for classes it compiles. If any of such event is dispatched after step 5 (when ClassPrepareEventListener is removed) there is no any registered listeners to handle it and this event is handled by the "unexpected events listener" (see above) that marks the test as failed.
That is why DefaultClassPrepareEventListener is needed: to process ClassPrepare events dispatched after ClassPrepareEventListener is unregistered inside testSourceFilter() method.
Please see below the new webrev with the changes you suggested.
Webrev: http://cr.openjdk.java.net/~dtitov/8204695/webrev.02/
Thanks!
Best regards,
Daniil
From: "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
Date: Tuesday, July 17, 2018 at 1:34 PM
To: Daniil Titov <daniil.x.titov at oracle.com>, "serviceability-dev at openjdk.java.net serviceability-dev at openjdk.java.net" <serviceability-dev at openjdk.java.net>
Subject: Re: RFR 8204695: [Graal] vmTestbase/nsk/jdi/ClassPrepareRequest/addSourceNameFilter/addSourceNameFilter002/addSourceNameFilter002.java fails
Hi Daniil,
Not sure, I fully understand the fix.
So, let's start from some questions.
Why the DefaultClassPrepareEventListener is needed?
Is it not enough to filter out the other threads in the
ClassPrepareEventListener.eventReceived() method ?
243 eventHandler.startListening();
244 // Add a listener to handle class prepared events fired by other ( e.g. compiler) threads.
245 // The listener should be added after the event listener is started to ensure that it called before
246 // the default event listener that handles unexpected events.
247 eventHandler.addListener(new DefaultClassPrepareEventListener());
It is still not clear why the default listener is added
after the listening is started but not before.
If the default listener is really needed then could you, please,
split the lines above and L129, L160 to make a little bit shorter?
I'd also suggest to replace "class prepared events" at L244
with "ClassPrepare event" or "class prepare event".
There is also an unneeded space in the "( e.g. compiler)".
Thanks,
Serguei
On 7/17/18 01:20, Daniil Titov wrote:
Please review the change that fix the JDI test when running with Graal.
The problem here is that the test verifies that a class prepare event is generated when the target VM loads a specific test class, but with Graal turned on additional class prepare events are generated by the compiler threads. The test doesn't expect them and fails. The fix ensures that additional class prepare events are ignored by the test and properly handled.
Bug: https://bugs.openjdk.java.net/browse/JDK-8204695
Webrev: http://cr.openjdk.java.net/~dtitov/8204695/webrev.01/
Thanks!
--Daniil
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180719/2cc490ec/attachment-0001.html>
More information about the serviceability-dev
mailing list