Discussion:
RFR (XS) 8214531: HeapMonitorEventOnOffTest.java test fails with "Statistics should be null to begin with"
JC Beyler
2018-11-30 17:08:48 UTC
Permalink
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/
Bug: https://bugs.openjdk.java.net/browse/JDK-8214531

Thanks,
Jc
s***@oracle.com
2018-11-30 19:12:48 UTC
Permalink
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi Jc,<br>
<br>
It looks good in general.<br>
I wonder if this comment is still needed:<br>
   // Enabling the notification system should start events again.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 11/30/18 09:08, JC Beyler wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF9BGBw-***@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">Hi all,
<div><br>
</div>
<div>Tiny webrev that removes enabling the sampling system
for the HeapMonitorEventOnOffTest before calling the <span>allocateAndCheckFrames</span> method
(the <span>allocateAndCheckFrames</span> method enables it
internally when no flag is passed to it).<br>
<div><br>
</div>
<div>Webrev: <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8214531/webrev.00/"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8214531/webrev.00/</a></div>
<div>Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8214531"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8214531</a></div>
<div dir="ltr" class="gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
</body>
</html>
JC Beyler
2018-11-30 19:48:03 UTC
Permalink
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
Post by s***@oracle.com
Hi Jc,
It looks good in general.
// Enabling the notification system should start events again.
Thanks,
Serguei
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/
Bug: https://bugs.openjdk.java.net/browse/JDK-8214531
Thanks,
Jc
--
Thanks,
Jc
s***@oracle.com
2018-11-30 20:22:42 UTC
Permalink
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi Jc,<br>
<br>
Looks better.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
<br>
On 11/30/18 11:48, JC Beyler wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF9BGBwt5k2+***@mail.gmail.com">
<div dir="ltr">Hi Serguei,<br>
<div><br>
</div>
<div>Technically "<span>allocateAndCheckFrames" does enable it
internally and the comment helps understand that we are
testing "sampling on - off - on", no?</span></div>
<div><span><br>
</span></div>
<div><span>I find that without the comments, you now have to
understand what </span><span>allocateAndCheckFrames does
implicitly.</span></div>
<div><span><br>
</span></div>
<div><span>We could change it to this:</span></div>
<div>   // By calling allocateAndCheckFrames(), we enabling the
notifications and check if allocations get sampled again<span><br>
</span></div>
<div><br>
</div>
<div>Does that look better?</div>
<div><br>
</div>
<div>Thanks!</div>
<div>Jc</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Fri, Nov 30, 2018 at 11:12 AM <a
href="mailto:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a> &lt;<a
href="mailto:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a>&gt;
wrote:<br>
</div>
<blockquote class="gmail_quote">
<div>
<div class="m_163393206688762065moz-cite-prefix">Hi Jc,<br>
<br>
It looks good in general.<br>
I wonder if this comment is still needed:<br>
   // Enabling the notification system should start events
again.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 11/30/18 09:08, JC Beyler wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">Hi all,
<div><br>
</div>
<div>Tiny webrev that removes enabling the sampling
system for the HeapMonitorEventOnOffTest before
calling the <span>allocateAndCheckFrames</span> method
(the <span>allocateAndCheckFrames</span> method
enables it internally when no flag is passed to
it).<br>
<div><br>
</div>
<div>Webrev: <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8214531/webrev.00/"
target="_blank" moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8214531/webrev.00/</a></div>
<div>Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8214531"
target="_blank" moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8214531</a></div>
<div dir="ltr"
class="m_163393206688762065gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
</div>
</blockquote>
</div>
<br>
<div><br>
</div>
-- <br>
<div dir="ltr" class="gmail_signature"
data-smartmail="gmail_signature">
<div dir="ltr">
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</blockquote>
<br>
</body>
</html>
JC Beyler
2018-11-30 21:53:34 UTC
Permalink
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
Post by s***@oracle.com
Hi Jc,
Looks better.
Thanks,
Serguei
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.
// By calling allocateAndCheckFrames(), we enabling the notifications
and check if allocations get sampled again
Does that look better?
Thanks!
Jc
Post by s***@oracle.com
Hi Jc,
It looks good in general.
// Enabling the notification system should start events again.
Thanks,
Serguei
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/
Bug: https://bugs.openjdk.java.net/browse/JDK-8214531
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
JC Beyler
2018-12-06 17:41:53 UTC
Permalink
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
Post by JC Beyler
Hi Serguei, thanks!
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
Post by s***@oracle.com
Hi Jc,
Looks better.
Thanks,
Serguei
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.
// By calling allocateAndCheckFrames(), we enabling the notifications
and check if allocations get sampled again
Does that look better?
Thanks!
Jc
Post by s***@oracle.com
Hi Jc,
It looks good in general.
// Enabling the notification system should start events again.
Thanks,
Serguei
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/
Bug: https://bugs.openjdk.java.net/browse/JDK-8214531
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
Hohensee, Paul
2018-12-06 18:54:49 UTC
Permalink
Lgtm.

Paul

From: serviceability-dev <serviceability-dev-***@openjdk.java.net> on behalf of JC Beyler <***@google.com>
Date: Thursday, December 6, 2018 at 9:42 AM
To: "***@oracle.com" <***@oracle.com>
Cc: "serviceability-***@openjdk.java.net" <serviceability-***@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 <***@google.com<mailto:***@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 ***@oracle.com<mailto:***@oracle.com> <***@oracle.com<mailto:***@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 ***@oracle.com<mailto:***@oracle.com> <***@oracle.com<mailto:***@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
Loading...