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.spit...@oracle.com" <serguei.spit...@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.spit...@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.spit...@oracle.com" <serguei.spit...@oracle.com>
>>> *Date: *Monday, December 3, 2018 at 4:14 PM
>>> *To: *Daniil Titov <daniil.x.ti...@oracle.com>, serviceability-dev
>>> <serviceability-dev@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 <jcbey...@google.com>
>>> *Date: *Friday, November 30, 2018 at 7:47 PM
>>> *To: *<daniil.x.ti...@oracle.com>
>>> *Cc: *<serviceability-dev@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
>>>
>>>
>>>
>>
