RFR 8214572: nsk/jvmti/unit/ForceEarlyReturn/earlyretbase should not suspend the thread when the top frame executes JVMCI code
David Holmes
david.holmes at oracle.com
Wed Dec 5 01:29:42 UTC 2018
Thanks Daniil, that seems fine to me.
I have some reservations about whether looping up to 100ms will
necessarily be enough for compilation to complete, but we can adjust in
the future as needed.
Cheers,
David
On 5/12/2018 11:15 am, Daniil Titov wrote:
> Thank you, David and Serguei,
>
> Please review an updated version of the patch with the corrected comment.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8214572
> Webrev: http://cr.openjdk.java.net/~dtitov/8214572/webrev.03
>
> Best regards,
> Daniil
>
> On 12/4/18, 4:55 PM, "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com> wrote:
>
>
>
> On 12/4/18 4:54 PM, David Holmes wrote:
> > Hi everyone,
> >
> > I'd actually argue that the comment not refer just to JVMCI but more
> > generally:
> >
> > + // when the top frame belongs to the test rather than to
> > incidental Java code (classloading, JVMCI, etc)
>
> Reasonable.
>
> > Also note typo: then -> than
>
> Nice catch!
>
> Thanks,
> Serguei
>
> >
> > Cheers,
> > David
> >
> > On 5/12/2018 5:40 am, serguei.spitsyn at oracle.com wrote:
> >> Hi Daniil,
> >>
> >> It looks good in general.
> >> Thank you for the update!
> >>
> >> I have some minor comment though.
> >>
> >> http://cr.openjdk.java.net/%7Edtitov/8214572/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.h.udiff.html
> >>
> >>
> >> +/**
> >> +* This method suspends the thread while ensuring the top frame
> >> executes the test method
> >> +* rather then JVMCI code triggered by invocation counter overflow.
> >> +*/
> >> +int suspendThreadAtMethod(jvmtiEnv *jvmti, jclass cls, jobject
> >> thread, jmethodID method);
> >>
> >>
> >> The comment above is not precise as it tells nothing about top frame.
> >>
> >> I like this one from implementation:
> >>
> >> + // We need to ensure that the thread is suspended at the right place
> >> + // when the top frame belongs to the test rather then to JVMCI code.
> >>
> >>
> >> So, the can be rephrased to something like:
> >>
> >> + // This method makes the thread to be suspended at the right place
> >> + // when the top frame belongs to the test rather then to JVMCI code.
> >>
> >>
> >>
> >> No need in another webrev if you fix the comment.
> >>
> >> Thanks,
> >> Serguei
> >>
> >>
> >> On 12/4/18 10:24 AM, Daniil Titov wrote:
> >>>
> >>> Hi Serguei and JC,
> >>>
> >>> Thank you for reviewing this change. And many thanks to David and
> >>> Dean for answering JVMCI questions.
> >>>
> >>> Please review a new version of the fix that moves the most of the
> >>> new code in a helper method ( as JC suggested) and corrects error
> >>> messages. I also excluded the changes in
> >>> test/hotspot/jtreg/ProblemList-graal.txt from this webrev.
> >>>
> >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8214572
> >>>
> >>> Webrev:http://cr.openjdk.java.net/~dtitov/8214572/webrev.02/
> >>> <http://cr.openjdk.java.net/%7Edtitov/8214572/webrev.02/>
> >>>
> >>> Thanks,
> >>>
> >>> --Daniil
> >>>
> >>> *From: *"serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
> >>> *Date: *Monday, December 3, 2018 at 4:14 PM
> >>> *To: *Daniil Titov <daniil.x.titov at oracle.com>, serviceability-dev
> >>> <serviceability-dev at openjdk.java.net>
> >>> *Subject: *Re: RFR 8214572:
> >>> nsk/jvmti/unit/ForceEarlyReturn/earlyretbase should not suspend the
> >>> thread when the top frame executes JVMCI code
> >>>
> >>> Hi Daniil,
> >>>
> >>> It looks good in general.
> >>>
> >>> I have two comments though.
> >>>
> >>> -vmTestbase/nsk/jvmti/unit/ForceEarlyReturn/earlyretbase/TestDescription.java
> >>> 8195635 generic-all
> >>>
> >>> It is not a good idea to remove the test from the ProblemList
> >>> before the 8195635 is fixed.
> >>>
> >>> 148 if(method == midActiveMethod) {
> >>> 149 printf("<<<<<<<< SuspendThread() is successfully
> >>> done\n");
> >>> 150 } else {
> >>> 151 printf("Warning: method \"activeMethod\" was missed\n");
> >>> 152 errCode = STATUS_FAILED;
> >>> 153 }
> >>>
> >>> I'd suggest to tweak the error message to something like:
> >>> "Failed in the suspThread: was not able to suspend thread with
> >>> the activeMethod() on top\n");
> >>>
> >>>
> >>> Thanks,
> >>> Serguei
> >>>
> >>> *From: *JC Beyler <jcbeyler at google.com>
> >>> *Date: *Friday, November 30, 2018 at 7:47 PM
> >>> *To: *<daniil.x.titov at oracle.com>
> >>> *Cc: *<serviceability-dev at openjdk.java.net>
> >>> *Subject: *Re: RFR 8214572:
> >>> nsk/jvmti/unit/ForceEarlyReturn/earlyretbase should not suspend the
> >>> thread when the top frame executes JVMCI code
> >>>
> >>> Hi Daniil,
> >>>
> >>> The webrev looks good but I have a few comments and questions (if
> >>> you don't mind :-)):
> >>>
> >>> Comments:
> >>>
> >>> - You say that normally the test will be removed from the problem
> >>> list once the two fixes are done but in this webrev, you've already
> >>> removed it (I can't see the other case so I can't see if it is
> >>> resolved :-))
> >>>
> >>> - now that we are in C++ for the tests, could we not declare the
> >>> variables at their first use instead of doing the pedantic top of
> >>> the block C style?
> >>>
> >>> - I feel that this sort of "wait until we are not in JVMCI frames"
> >>> might happen a lot, maybe we could move that code into a helper
> >>> method (+ it cleans the method a bit to just concentrate on the
> >>> rest) and then if needed we can make it public to other tests?
> >>>
> >>> Questions because I'm not familiar with JVMCI consequences so not
> >>> really comments on the webrev but so that I know:
> >>>
> >>> - Is it normaly that you can suspend when you are in a JVMCI
> >>> frame? will/is there not a better way that we could detect that we
> >>> are in a JVMCI frame? Is it always safe to suspend a JVMCI frame?
> >>>
> >>> Thanks!
> >>>
> >>> Jc
> >>>
> >>>
> >>>
> >>>
> >>> On 11/30/18 19:08, Daniil Titov wrote:
> >>>
> >>> Please review the change for
> >>> nsk/jvmti/unit/ForceEarlyReturn/earlyretbase test. The problem here
> >>> is that before doing an early force return the test doesn't check
> >>> that the test thread is suspended at a right place where the top
> >>> frame executes the test method rather than JVMCI code triggered by
> >>> invocation counter overflow. That results in the early return
> >>> happens for a wrong method and the test fails.
> >>>
> >>> The fix changes the test to do resume/suspend in the loop until
> >>> the target method is executed in the top frame or the loop counter
> >>> exceeds the limit.
> >>>
> >>> There is another problem with this test that requires changes on
> >>> VM side and is currently covered by JDK-8195635:" [Graal]
> >>> nsk/jvmti/unit/ForceEarlyReturn/earlyretbase crashes with assertion
> >>> "compilation level out of bounds"". The test will have to be
> >>> removed from test/hotspot/jtreg/ProblemList-graal.txt only after
> >>> both these issues are fixed.
> >>>
> >>> Bug:https://bugs.openjdk.java.net/browse/JDK-8214572
> >>>
> >>> Webrev:http://cr.openjdk.java.net/~dtitov/8214572/webrev.01/
> >>> <http://cr.openjdk.java.net/%7Edtitov/8214572/webrev.01/>
> >>> Thanks,
> >>>
> >>> Daniil
> >>>
> >>>
> >>>
> >>
>
>
>
>
More information about the serviceability-dev
mailing list