I just tried the suggestion to add the check after thread1 join call
and it fails with :
Thread thread1: invalid thread group
If a thread completes and we join the thread, not all information
of the terminated thread is available.
I think we need to limit the test checking to before starting t
On 08/27/2018 05:08, Gary Adams wrote:
I just tried the suggestion to add the check after thread1 join call
and it fails with :
Thread thread1: invalid thread group
If a thread completes and we join the thread, not all information
of the terminated thread is available.
Per spec after thr
You are right! I had used the original thread group and missed the
need to refetch getThreadGroup().
Revised changeset:
diff --git
a/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetThreadInfo/thrinfo001.java
b/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetThreadInfo/thrinfo001.java
--- a/test/hotspot
Hi Serguei,
Fair enough, at least this removes a bit of the chance of flakiness :-)
Should we at least clean up the comment for methods that are changed?
/**
* Testcase: check tested threads
*- invoke getFrameCount() for each thread
*- check if frameCount is not less than minimal stac
Looks good to me.
--alex
On 08/27/2018 10:30, Gary Adams wrote:
You are right! I had used the original thread group and missed the
need to refetch getThreadGroup().
Revised changeset:
diff --git
a/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetThreadInfo/thrinfo001.java
b/test/hotspot/jtreg/vmT
Hi Jc,
Thank you for reviewing this change. I had the similar concerns initially
regarding the losing coverage in the general case and was thinking about
detecting the Graal frames and adjusting the effective stack depth to not count
them. However, I found this approach a quite cumbersome an
Hi Jc,
Thank you for spotting this! I will send on review an updated webrev with
these comments fixed.
Best regards,
Daniil
From: JC Beyler
Date: Monday, August 27, 2018 at 10:41 AM
To:
Cc: ,
Subject: Re: RFR: 8209585: [Graal] vmTestbase/nsk/jvmti/scenarios/sampling
tests fail wi
+1
Chris
On 8/27/18 10:46 AM, Alex Menkov wrote:
Looks good to me.
--alex
On 08/27/2018 10:30, Gary Adams wrote:
You are right! I had used the original thread group and missed the
need to refetch getThreadGroup().
Revised changeset:
diff --git
a/test/hotspot/jtreg/vmTestbase/nsk/jvmti/Get
Hi Gary,
Just getting caught up again. To answer your earlier question, yes, I
think removing the isDisconnected() check is an improvement since it's
use was at best inconsistent, and leads the reader to think that this is
something that is expected to happen. If it does happen, the test will
Hi Alex,
Changes look good.
thanks,
Chris
On 8/24/18 11:54 AM, Alex Menkov wrote:
Hi all,
Please review a fix for
https://bugs.openjdk.java.net/browse/JDK-8203393
webrev:
http://cr.openjdk.java.net/~amenkov/sh2java/jdbSolarisTimeout/webrev/
The fix converts the test to java (and this resolv
Hi Alex,
I noticed you no longer strip the trailing newline from JdbCommand. Are
you sure when we print the command we are seeing the proper output of
newlines (no extra ones). For example, we have the following in Jdb.java:
System.out.println("> " + cmd.cmd);
inputWriter.pr
http://cr.openjdk.java.net/~iignatyev//8186548/webrev.00/index.html
> 10 lines changed: 4 ins; 4 del; 2 mod;
Hi all,
could you please review this small patch which puts JcmdBase into the directory
w/ the only two tests which use it?
JBS: https://bugs.openjdk.java.net/browse/JDK-8186548
webrev:
Hi JC,
The jvmtiExport.cpp changes look fine, but I'm no expert in this
area.
I think you need to fix the copyrights in the new files. My
understanding is they need to include the Oracle copyright. Search
for examples from "Red Hat" and "SAP" t
Looks good.
Chris
On 8/27/18 3:37 PM, Igor Ignatyev wrote:
http://cr.openjdk.java.net/~iignatyev//8186548/webrev.00/index.html
10 lines changed: 4 ins; 4 del; 2 mod;
Hi all,
could you please review this small patch which puts JcmdBase into the directory
w/ the only two tests which use it?
Hi Chris,
This "newline stripping" logic was copied from nsk classes, where jdb
commands are represented as string constants and some of the constants
have newline at the end (for the commands which do not have arguments),
and others don't have.
JdbCommand class in com/sun/jdi/lib/jdb/ has sta
+1
--alex
On 08/27/2018 15:41, Chris Plummer wrote:
Looks good.
Chris
On 8/27/18 3:37 PM, Igor Ignatyev wrote:
http://cr.openjdk.java.net/~iignatyev//8186548/webrev.00/index.html
10 lines changed: 4 ins; 4 del; 2 mod;
Hi all,
could you please review this small patch which puts JcmdBase in
Yeah, I think the issue I had with extra newlines was with nsk tests.
But it was something you only saw if the test failed, forcing the log to
include all the output.
thanks,
Chris
On 8/27/18 4:14 PM, Alex Menkov wrote:
Hi Chris,
This "newline stripping" logic was copied from nsk classes, w
yes, I understand.
To verify the output I added throw at the end of the test :)
--alex
On 08/27/2018 16:36, Chris Plummer wrote:
Yeah, I think the issue I had with extra newlines was with nsk tests.
But it was something you only saw if the test failed, forcing the log to
include all the output
Hi Alex and Chris,
thanks for your reviews.
-- Igor
> On Aug 27, 2018, at 4:16 PM, Alex Menkov wrote:
>
> +1
>
> --alex
>
> On 08/27/2018 15:41, Chris Plummer wrote:
>> Looks good.
>> Chris
>> On 8/27/18 3:37 PM, Igor Ignatyev wrote:
>>> http://cr.openjdk.java.net/~iignatyev//8186548/webrev.
http://cr.openjdk.java.net/~iignatyev//8210022/webrev.00/index.html
> 303 lines changed: 21 ins; 266 del; 16 mod;
Hi all,
could you please review the testlibrary clean up which removes
jdk.testlibrary.ProcessThread, TestThread and XRun classes and update all their
users to use the same classes
Hi JC, Serguei, and Alex,
Please review an updated version of the webrev that has these comments fixed.
Webrev: http://cr.openjdk.java.net/~dtitov/8209585/webrev.02/
Issue: https://bugs.openjdk.java.net/browse/JDK-8209585
Thanks!
Daniil
From: serviceability-dev on
behalf of Daniil Titov
D
Hi Daniil,
(Not a reviewer for reference) but looks good to me :)
Thanks for fixing the comments!
Jc
On Mon, Aug 27, 2018 at 5:57 PM Daniil Titov
wrote:
> Hi JC, Serguei, and Alex,
>
> Please review an updated version of the webrev that has these comments
> fixed.
>
> Webrev: http://cr.openjdk
(cc'ing compiler alias)
I ain't sure that we should change these tests given that w/ libgraal (as an
opposite to the current way we use graal) we shouldn't see this issue at all.
I'd rather left these tests in the graal-specific problem list till we switch
to libraal.
Vladimir, Katja, what do
That was my first reaction when looking at the explanation, but then I
realized the test is testing that the stack sizes are the same for two
different snapshots of the same */unsuspended/* thread. That sounds like
reason enough to make the change, even if graal were not an issue.
thanks,
Chr
+1
Thanks,
Serguei
On 8/27/18 13:04, Chris Plummer wrote:
+1
Chris
On 8/27/18 10:46 AM, Alex Menkov wrote:
Looks good to me.
--alex
On 08/27/2018 10:30, Gary Adams wrote:
You are right! I had used the original thread group and missed the
need to refetch getThreadGroup().
Revised changese
+1
Thanks,
Serguei
On 8/27/18 16:16, Alex Menkov wrote:
+1
--alex
On 08/27/2018 15:41, Chris Plummer wrote:
Looks good.
Chris
On 8/27/18 3:37 PM, Igor Ignatyev wrote:
http://cr.openjdk.java.net/~iignatyev//8186548/webrev.00/index.html
10 lines changed: 4 ins; 4 del; 2 mod;
Hi all,
coul
Hi Igor,
It looks good.
The copyright comment needs an update:
http://cr.openjdk.java.net/~iignatyev//8210022/webrev.00/test/jdk/com/sun/tools/attach/ProviderTest.java.frames.html
Thanks,
Serguei
On 8/27/18 17:32, Igor Ignatyev wrote:
http://cr.openjdk.java.net/~iignatyev//8210022/webrev.00/i
Totally agree.
Thanks,
Serguei
On 8/27/18 19:14, Chris Plummer wrote:
That was my first reaction when looking at the explanation, but then I
realized the test is testing that the stack sizes are the same for two
different snapshots of the same */unsuspended/* thread. That sounds
like reason e
Hi Daniil and Jc,
I'm thinking if it make sense to call checkThreads() when the threads
are not suspended.
In fact, it does not check much for non-suspended threads now.
As an example, consider the test:
http://cr.openjdk.java.net/~dtitov/8209585/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jv
Hi Chris,
Thanks for looking at the webrev. I fixed the copyrights for the files here
and also created https://bugs.openjdk.java.net/browse/JDK-8210035 because I
saw that files I created for the HeapMonitor work have the same issue. I'll
send out a webrev shortly to fix those.
Thanks again!
Jc
O
30 matches
Mail list logo
