Hi Serguei,

On 6/12/18 1:08 PM, serguei.spit...@oracle.com wrote:
Hi Daniil,


On 6/12/18 12:30, Daniil Titov wrote:

Hi Serguei and Chris,

As I mentioned in the previous emails there are only 4 other tests that calls EventFiliters.filtered(event) and only 2 of them need to be changed to use  EventFiliters.filtered(event, debuggeName).

So if the approach in the first webrev http://cr.openjdk.java.net/~dtitov/8203809/webrev.02/ <http://cr.openjdk.java.net/%7Edtitov/8203809/webrev.02/> is preferable


I guess, it is just a typo above.
It is not the first approach but the second.

It is a typo. The proper link is http://cr.openjdk.java.net/~dtitov/8203809/webrev.01/


we could stick with it and I will just update this patch to add 2 more tests and combine EventFiliters.filtered(event, typeName) and EventFiliters.filteredByLocation(event, typeName) in the single method as Serguei suggested.


Yes, my preference is this approach.
It adds some ground for future improvements as well.
It is also possible to use this method for outer classes being debugged like this:
filtered(event, "java.lang.Integer")

Thanks,
Serguei

Agree. Will prepare an updated webrev soon.

Thanks!

Thanks!

Best regards,

Daniil

*From: *"serguei.spit...@oracle.com" <serguei.spit...@oracle.com>
*Date: *Tuesday, June 12, 2018 at 12:21 PM
*To: *Chris Plummer <chris.plum...@oracle.com>, Daniil Titov <daniil.x.ti...@oracle.com>, "serviceability-dev@openjdk.java.net serviceability-dev@openjdk.java.net" <serviceability-dev@openjdk.java.net> *Subject: *Re: RFR 8203809: [Graal] JDI tests fail with: JDITestRuntimeException : ** event IS NOT a breakpoint **



On 6/12/18 12:02, Chris Plummer wrote:

    On 6/12/18 11:53 AM, serguei.spit...@oracle.com
    <mailto:serguei.spit...@oracle.com> wrote:

        Hi Daniil,

        I don't think, the approach for the tests to filter out the
        JFR and Graal threads
        is not good despite the fact it is relatively easy to implement.
        It does not cover any threads that may come in the future.
        So that I consider the webrev below as just a work around of
        the real problem.

    True, but his changes are just an extension of what is already
    being done - filtering out threads we don't care about.


I understand it.


        The tests should just filter out everything that is not a
        part of their testing.
        It can be done on a base of the Debuggee classes or threads,
        or any other approaches.

    One problem is that events don't always come in on debuggee
    thread, so you can't filter out all threads other than the
    debuggee thread. I think probably filtering out all classes not
    related to the debuggee is probably the way to go. However, do we
    want to fix all the tests to do this, or just go with Daniil's
    quick fix?


In general, I'm Okay with this quick workaround fix.
But wanted to underline that we still have a base for problems in the future.

Thanks,
Serguei



    Chris


        Thanks,
        Serguei


        On 6/12/18 10:57, Daniil Titov wrote:

            Hi Serguei,

            There are 4 more tests that use EventFilters.filter()
            method and not all of them need to be changed to call
            EventFilters.filtered(Event event, String typeName).
            Specifically,
            
vmTestbase/nsk/jdi/ExceptionEvent/exception/exception001/TestDescription.java
            and
            
vmTestbase/nsk/jdi/ExceptionEvent/catchLocation/location001/TestDescription.java
            tests expect to receive an ExceptionEvent for
            NumberFormatException exception and the location of this
            event happens to be not the debuggee class (
            java.lang.Integer:652). The tests pass now and changing
            them to use EventFilters.filtered(event, debugeeName)
            makes them fail.

            There is also an alternative approach I would like to
            suggest. All these new Graal events are generated by the
            compiler threads (“JVMCI CompilerThread0, JVMCI
            CompilerThread1, etc.”) and could be filtered out inside
            EventFilters.filtered(Event event) method by checking the
            thread reference for the given event object.

            Please review the updated fix the uses this approach.

            Webrev:
            http://cr.openjdk.java.net/~dtitov/8203809/webrev.02/
            <http://cr.openjdk.java.net/%7Edtitov/8203809/webrev.02/>

            Bug: https://bugs.openjdk.java.net/browse/JDK-8203809

            Thanks!

            Best regards,

            Daniil

            *From: *"serguei.spit...@oracle.com"
            <mailto:serguei.spit...@oracle.com>
            <serguei.spit...@oracle.com>
            <mailto:serguei.spit...@oracle.com>
            *Date: *Monday, June 11, 2018 at 6:18 PM
            *To: *Daniil Titov <daniil.x.ti...@oracle.com>
            <mailto:daniil.x.ti...@oracle.com>,
            "serviceability-dev@openjdk.java.net
            serviceability-dev@openjdk.java.net"
            
<mailto:serviceability-...@openjdk.java.netserviceability-...@openjdk.java.net>
            <serviceability-dev@openjdk.java.net>
            <mailto:serviceability-dev@openjdk.java.net>
            *Subject: *Re: RFR 8203809: [Graal] JDI tests fail with:
            JDITestRuntimeException : ** event IS NOT a breakpoint **

            Hi Daniil,

            It looks good in general.
            Some minor comments though.

            
http://cr.openjdk.java.net/~dtitov/8203809/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/share/jdi/EventFilters.java.udiff.html
            
<http://cr.openjdk.java.net/%7Edtitov/8203809/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/share/jdi/EventFilters.java.udiff.html>

            +

            +    // Filters out events with location not matching the
            given type.

            +    public static boolean filteredByLocation(Event
            event, String typeName) {

            +        if (event instanceof Locatable) {

            +            Location location = ((Locatable)
            event).location();

            +            if (location != null) {

            +                ReferenceType declaringType =
            location.declaringType();

            +                if (declaringType != null &&
            typeName.equals(declaringType.name())) {

            +                    return false;

            +                }

            +            }

            +        }

            +        return true;

            +    }

            +

            +    // Filters out internal JFR events and events with
            location not matching

            +    // the given type.

            +    public static boolean filtered(Event event, String
            typeName) {

            +

            +        return filtered(event) ||
            filteredByLocation(event,typeName);

            +

            +    }

            +

            +

            There are a couple of extra empty lines.

            I'm suggesting to get rid of the second method and rename
            filteredByLocation() to filtered():

            +    // Filters out events with location not matching the
            given type.

            +    public static boolean filtered(Event event, String
            typeName) {

            +        if (event instanceof Locatable) {

            +            Location location = ((Locatable)
            event).location();

            +            if (location != null) {

            +                ReferenceType declaringType =
            location.declaringType();

            +                if (declaringType != null &&
            typeName.equals(declaringType.name())) {

            +                    return false;

            +                }

            +            }

            +        }

            +        return true;

            +    }


            As I understand, it should filter out the events post on
            the JFR threads.
            Please, correct me if I'm wrong.
            My view is that we may want to get rid of the
            filtered(Event event) method and
            convert all the tests to use the filtered(Event event,
            String typeName) instead.

            Thank you a lot for taking care about these issues!

            Thanks,
            Serguei


            On 6/11/18 17:52, Daniil Titov wrote:

                Please review the changes that fix failure of 6 JDI tests run 
with Graal.

                The problem here is that currently the tests filter out only 2 
specific JFR events (nsk.share.jdi.EventsFilter.filtered()) before analyzing 
them and fail if the received event doesn't match the expected one.  But with 
Graal turned on new events generated by the compiler thread are occasionally 
found in the event queue and cause the tests failure. The fix makes the tests 
to use an additional check to filter out events not related to the debuggee 
class.

Bug:https://bugs.openjdk.java.net/browse/JDK-8203809
                Webrev:http://cr.openjdk.java.net/~dtitov/8203809/webrev.01
                <http://cr.openjdk.java.net/%7Edtitov/8203809/webrev.01>

                Thanks,

                Daniil





Reply via email to