RFR (XS) 8214531: HeapMonitorEventOnOffTest.java test fails with "Statistics should be null to begin with"

[Hohensee, Paul]

Lgtm.

Paul

From: serviceability-dev <serviceability-dev-bounces at openjdk.java.net> on behalf of JC Beyler <jcbeyler at google.com>
Date: Thursday, December 6, 2018 at 9:42 AM
To: "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
Cc: "serviceability-dev at openjdk.java.net" <serviceability-dev at openjdk.java.net>
Subject: Re: RFR (XS) 8214531: HeapMonitorEventOnOffTest.java test fails with "Statistics should be null to begin with"

Hi all,

Anyway I could get a second review on this please? :)
Webrev: http://cr.openjdk.java.net/~jcbeyler/8214531/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8214531

Thanks!
Jc

On Fri, Nov 30, 2018 at 1:53 PM JC Beyler <jcbeyler at google.com<mailto:jcbeyler at google.com>> wrote:
Hi Serguei, thanks!

Done here then:

Webrev: http://cr.openjdk.java.net/~jcbeyler/8214531/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8214531

I've sent it to the submit repo while waiting for a second review :)

Have a great friday!
Jc

On Fri, Nov 30, 2018 at 12:22 PM serguei.spitsyn at oracle.com<mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com<mailto:serguei.spitsyn at oracle.com>> wrote:
Hi Jc,

Looks better.

Thanks,
Serguei



On 11/30/18 11:48, JC Beyler wrote:
Hi Serguei,

Technically "allocateAndCheckFrames" does enable it internally and the comment helps understand that we are testing "sampling on - off - on", no?

I find that without the comments, you now have to understand what allocateAndCheckFrames does implicitly.

We could change it to this:
   // By calling allocateAndCheckFrames(), we enabling the notifications and check if allocations get sampled again

Does that look better?

Thanks!
Jc

On Fri, Nov 30, 2018 at 11:12 AM serguei.spitsyn at oracle.com<mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com<mailto:serguei.spitsyn at oracle.com>> wrote:
Hi Jc,

It looks good in general.
I wonder if this comment is still needed:
   // Enabling the notification system should start events again.

Thanks,
Serguei


On 11/30/18 09:08, JC Beyler wrote:
Hi all,

Tiny webrev that removes enabling the sampling system for the HeapMonitorEventOnOffTest before calling the allocateAndCheckFrames method (the allocateAndCheckFrames method enables it internally when no flag is passed to it).

Webrev: http://cr.openjdk.java.net/~jcbeyler/8214531/webrev.00/<http://cr.openjdk.java.net/%7Ejcbeyler/8214531/webrev.00/>
Bug: https://bugs.openjdk.java.net/browse/JDK-8214531

Thanks,
Jc



--

Thanks,
Jc



--

Thanks,
Jc


--

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181206/f4e21a52/attachment.html>