RFR: 8190353: [Testbug] test/hotspot/jtreg/gc/logging/TestPrintReferences.java can still fail

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8190353: [Testbug] test/hotspot/jtreg/gc/logging/TestPrintReferences.java can still fail

Stefan Johansson
Hi,

Please review this fix for:
https://bugs.openjdk.java.net/browse/JDK-8190353

Webrev:
http://cr.openjdk.java.net/~sjohanss/8190353/00/

Summary:
The fix for JDK-8188245 was incomplete, it removed the check that if the
reported time is longer than the sum of the sub-phases everything is ok.
The new code requires the sum of the sub-phases to be within a give
tolerance from the reported time. This is true in many cases but we
can't know for sure that we won't get a context switch or some other
stall between two sub-phases that causes the reported total time to be
longer, but the sub-phases to not include this extra time.

Testing:
Verified that the new verification method returns correct values for
different input values.

Thanks,
Stefan
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8190353: [Testbug] test/hotspot/jtreg/gc/logging/TestPrintReferences.java can still fail

sangheon.kim@oracle.com
Hi Stefan,

Thank you for finding and fixing this bug.

On 10/30/2017 08:04 AM, Stefan Johansson wrote:
Hi,

Please review this fix for:
https://bugs.openjdk.java.net/browse/JDK-8190353

Webrev:
http://cr.openjdk.java.net/~sjohanss/8190353/00/

Summary:
The fix for JDK-8188245 was incomplete, it removed the check that if the reported time is longer than the sum of the sub-phases everything is ok. The new code requires the sum of the sub-phases to be within a give tolerance from the reported time. This is true in many cases but we can't know for sure that we won't get a context switch or some other stall between two sub-phases that causes the reported total time to be longer, but the sub-phases to not include this extra time.

Testing:
Verified that the new verification method returns correct values for different input values.
Looks good.

Just minor nit: you can ignore or no webrev is needed.
140   // Because of this we need method to verify that our measurements and calculations are valid.
-> Because of this, we need a method ...

Thanks,
Sangheon



Thanks,
Stefan

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8190353: [Testbug] test/hotspot/jtreg/gc/logging/TestPrintReferences.java can still fail

Erik Osterlund
In reply to this post by Stefan Johansson
Hi Stefan,

Looks good.

/Erik

> On 30 Oct 2017, at 16:04, Stefan Johansson <[hidden email]> wrote:
>
> Hi,
>
> Please review this fix for:
> https://bugs.openjdk.java.net/browse/JDK-8190353
>
> Webrev:
> http://cr.openjdk.java.net/~sjohanss/8190353/00/
>
> Summary:
> The fix for JDK-8188245 was incomplete, it removed the check that if the reported time is longer than the sum of the sub-phases everything is ok. The new code requires the sum of the sub-phases to be within a give tolerance from the reported time. This is true in many cases but we can't know for sure that we won't get a context switch or some other stall between two sub-phases that causes the reported total time to be longer, but the sub-phases to not include this extra time.
>
> Testing:
> Verified that the new verification method returns correct values for different input values.
>
> Thanks,
> Stefan