<html xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:0in;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
span.EmailStyle18
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style>
</head>
<body lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal">Looks good to me.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Paul<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:12.0pt;color:black">From: </span></b><span style="font-size:12.0pt;color:black">JC Beyler <jcbeyler@google.com><br>
<b>Date: </b>Thursday, October 11, 2018 at 6:38 PM<br>
<b>To: </b>David Holmes <david.holmes@oracle.com><br>
<b>Cc: </b>"Hohensee, Paul" <hohensee@amazon.com>, "serviceability-dev@openjdk.java.net" <serviceability-dev@openjdk.java.net><br>
<b>Subject: </b>Re: RFR (S) 8211980: Remove ThreadHeapSampler enable/disable/enabled methods<o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Hi all, <o:p></o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">@David: I did your two requests for volatile and removing the lock<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">The latest webrev is now:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">Webrev: <a href="http://cr.openjdk.java.net/~jcbeyler/8211980/webrev.02" target="_blank">http://cr.openjdk.java.net/~jcbeyler/8211980/webrev.02</a><o:p></o:p></p>
</div>
<div>
<div>
<p class="MsoNormal">Bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8211980" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8211980</a><o:p></o:p></p>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<p class="MsoNormal">Thanks,<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">Jc<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">On Thu, Oct 11, 2018 at 3:24 PM David Holmes <<a href="mailto:david.holmes@oracle.com">david.holmes@oracle.com</a>> wrote:<o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal" style="margin-bottom:12.0pt">On 12/10/2018 8:20 AM, JC Beyler wrote:<br>
> I'm 100% in agreement with David :)<br>
> <br>
> Inlined are my comments:<br>
> <br>
> On Thu, Oct 11, 2018 at 3:13 PM David Holmes <<a href="mailto:david.holmes@oracle.com" target="_blank">david.holmes@oracle.com</a>
<br>
> <mailto:<a href="mailto:david.holmes@oracle.com" target="_blank">david.holmes@oracle.com</a>>> wrote:<br>
> <br>
> On 12/10/2018 3:44 AM, Hohensee, Paul wrote:<br>
> > So, given that the lock was only used to protect log_table<br>
> initialization, and that the patch moves that into the C++ "class<br>
> initializer" which is run when the first Thread object is<br>
> constructed, we don't need any locking/memory ordering anymore, right?<br>
> <br>
> Right so:<br>
> <br>
> - ThreadHeapSampler_lock can be removed<br>
> - The load-acquire/release-store of _sampling_interval seem to serve no<br>
> purpose, but _sampling_interval should at least be marked volatile<br>
> (from<br>
> a documentation perspective if nothing else). If the intent was for a<br>
> change in sampling_interval to be immediately visible then a fence()<br>
> would be needed.<br>
> <br>
> <br>
> Right, I would leave them in place (or at least postpone the <br>
> conversation about them to a later webrev if someone feels really <br>
> strongly about them; I believe it does not hurt, this will never be <br>
> critical code but at least is semantically safe).<br>
<br>
Please deleted the unused ThreadHeapSampler_lock.<br>
<br>
Please mark _sampling_interval as volatile.<o:p></o:p></p>
</blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal">> <br>
> The _log_table_initialized flag is not needed from the perspective<br>
> of an<br>
> initialization check - you can't run code until after the static<br>
> initializers have run (though I'm unclear how C++ manages this from a<br>
> concurrency perspective). But the flag may be needed just as a means to<br>
> separate the allocation of the table from the initialization of it -<br>
> again I'm unclear how C++ static initialization works in detail (you<br>
> have to be very careful with such initialization to ensure there are<br>
> zero dependencies on anything done as part of VM initialization).<br>
> <br>
> <br>
> Exactly. I cannot force the initialization of a static array via a <br>
> method without initialization *something*. It can be to initialize a <br>
> pointer to the array and I just use the pointer in the class; that was <br>
> what the first webrev one was doing. Using a boolean such as here seems <br>
> to be the lesser evil and allows us to add an assert that all is well in <br>
> the world at the only usage point of the array in assert mode.<br>
> <br>
> So I'm happy with the current form (I'm biased since I sent the webrev <br>
> for review :-)), any LGTM or other comments?<br>
<br>
I can't comment on the details of the code just the general structural <br>
changes, and they seem okay to me.<br>
<br>
Thanks,<br>
David<br>
<br>
> Jc<br>
> <br>
> David<br>
> <br>
> > Paul<br>
> ><br>
> > On 10/11/18, 4:11 AM, "David Holmes" <<a href="mailto:david.holmes@oracle.com" target="_blank">david.holmes@oracle.com</a><br>
> <mailto:<a href="mailto:david.holmes@oracle.com" target="_blank">david.holmes@oracle.com</a>>> wrote:<br>
> ><br>
> > On 11/10/2018 3:10 AM, Hohensee, Paul wrote:<br>
> > > There used to be a rule against using “namespaceâ€. Don’t<br>
> know if that’s<br>
> > > still true or not: I believe it is. Hence the “static<br>
> const <whatever>â€.<br>
> > ><br>
> > > You only need OrderAccess if there could be a race on<br>
> accesses to<br>
> > > whatever you’re guarding. Looks like the old code wanted<br>
> to make sure<br>
> > > that log_table was initialized and its content available<br>
> to other<br>
> > > threads before the _enabled flag was set. I.e., the<br>
> _enabled flag acted<br>
> > > as a store visibility barrier for log_table, and possibly<br>
> other<br>
> > > ThreadHeapSampler data structures. If no thread can ever<br>
> see a partially<br>
> > > initialized log_table, then no need for ordering. The old<br>
> code used a<br>
> > > MutexLocker in init_log_table(), which former has a fence<br>
> in its<br>
> > > destructor and probably (haven’t dived into the code)<br>
> guards all<br>
> > > accesses to log_table, so the release_store() on _enabled<br>
> in enable()<br>
> > > was redundant.<br>
> ><br>
> > The release_store and load_acquire were necessary to ensure the<br>
> > lock-free enabled() check ensured visibility of the<br>
> initialization of<br>
> > the data structures in the ensuing code. Otherwise you'd<br>
> need to grab<br>
> > the lock on the enabled() checks, which is too heavy-weight.<br>
> The lock is<br>
> > only used to ensure single-threaded initialization of the<br>
> log_table,<br>
> > actual accesses are again lock-free.<br>
> ><br>
> > David<br>
> ><br>
> > Same with the release_store() in disable(), unless there<br>
> > > was some reason to make sure all threads saw previous<br>
> stores to<br>
> > > ThreadHeapSampler related memory before _enabled was set<br>
> to zero. The<br>
> > > load_acquire in enabled() may not have been needed either,<br>
> because it<br>
> > > only prevents subsequent loads from being executed before<br>
> the load from<br>
> > > _enabled, so if _enabled was being used to guard access<br>
> only to<br>
> > > ThreadHeapSampler data such as log_table, the<br>
> release_store() on<br>
> > > _enabled would guarantee that all necessary stores would<br>
> be done before<br>
> > > _enabled was set to one and seen by enabled().<br>
> > ><br>
> > > Yeah, that’s hard to follow, and I wrote it. :) It comes<br>
> down to what<br>
> > > you’re guarding with OrderAccess. If it’s only<br>
> ThreadHeapSampler data,<br>
> > > and since only a Thread has one, and since<br>
> ThreadHeapSampler statics are<br>
> > > initialized before construction of the first<br>
> _heap_sampler, and since<br>
> > > the construction of a Thread is guarded by multiple<br>
> mutexes which will<br>
> > > force visibility of any ThreadHeapSampler statics before a<br>
> Thread is<br>
> > > used, you don’t need OrderAccess.<br>
> > ><br>
> > > I’d put anything to do with ThreadHeapSampler into its<br>
> class definition<br>
> > > rather than define them at file scope in<br>
> threadHeapSampler.cpp. I.e.,<br>
> > > all of FastLogNumBits, FastLogMask, and internal_log_table<br>
> (and name it<br>
> > > back to that log_table). File scope data is a no-no.<br>
> > ><br>
> > > Hope this helps,<br>
> > ><br>
> > > Paul<br>
> > ><br>
> > > *From: *serviceability-dev<br>
> <<a href="mailto:serviceability-dev-bounces@openjdk.java.net" target="_blank">serviceability-dev-bounces@openjdk.java.net</a><br>
> <mailto:<a href="mailto:serviceability-dev-bounces@openjdk.java.net" target="_blank">serviceability-dev-bounces@openjdk.java.net</a>>><br>
> > > on behalf of JC Beyler <<a href="mailto:jcbeyler@google.com" target="_blank">jcbeyler@google.com</a><br>
> <mailto:<a href="mailto:jcbeyler@google.com" target="_blank">jcbeyler@google.com</a>>><br>
> > > *Date: *Tuesday, October 9, 2018 at 11:58 PM<br>
> > > *To: *"<a href="mailto:serviceability-dev@openjdk.java.net" target="_blank">serviceability-dev@openjdk.java.net</a><br>
> <mailto:<a href="mailto:serviceability-dev@openjdk.java.net" target="_blank">serviceability-dev@openjdk.java.net</a>>"<br>
> > > <<a href="mailto:serviceability-dev@openjdk.java.net" target="_blank">serviceability-dev@openjdk.java.net</a><br>
> <mailto:<a href="mailto:serviceability-dev@openjdk.java.net" target="_blank">serviceability-dev@openjdk.java.net</a>>><br>
> > > *Subject: *RFR (S) 8211980: Remove ThreadHeapSampler<br>
> > > enable/disable/enabled methods<br>
> > ><br>
> > > Hi all,<br>
> > ><br>
> > > When talking with Serguei about JDK-8201655<br>
> > > <<a href="https://bugs.openjdk.java.net/browse/JDK-8201655" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8201655</a>>, we<br>
> talked about why<br>
> > > ThreadHeapSampler has an enabled/disabled when we could<br>
> have just used<br>
> > > the should_post_sampled_object_alloc to begin with.<br>
> > ><br>
> > > Could I get a review for this:<br>
> > ><br>
> > > Webrev:<br>
> <a href="http://cr.openjdk.java.net/~jcbeyler/8211980/webrev.00/" target="_blank">http://cr.openjdk.java.net/~jcbeyler/8211980/webrev.00/</a><br>
> <<a href="http://cr.openjdk.java.net/%7Ejcbeyler/8211980/webrev.00/" target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8211980/webrev.00/</a>><br>
> > > <<a href="http://cr.openjdk.java.net/%7Ejcbeyler/8211980/webrev.00/" target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8211980/webrev.00/</a>><br>
> > ><br>
> > > Bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8211980" target="_blank">
https://bugs.openjdk.java.net/browse/JDK-8211980</a><br>
> > ><br>
> > > This passed my testing on my dev machine in release and<br>
> fastdebug.<br>
> > ><br>
> > > The question I would like to raise here at the same time<br>
> (in order to<br>
> > > reduce email spam and because it should be included in the<br>
> review I<br>
> > > believe) is:<br>
> > ><br>
> > > - When I did the enable/disable, I used OrderAccess to<br>
> do so after a<br>
> > > reviewer asked for it<br>
> > ><br>
> > > - From what I can tell, JVMTI_SUPPORT_FLAG does not use<br>
> it and does<br>
> > > instead:<br>
> > ><br>
> > > #define JVMTI_SUPPORT_FLAG(key) <br>
> \<br>
> > ><br>
> > > private: <br>
> \<br>
> > ><br>
> > > static bool _##key; <br>
> \<br>
> > ><br>
> > > public: <br>
> \<br>
> > ><br>
> > > inline static void set_##key(bool on) { <br>
> \<br>
> > ><br>
> > > JVMTI_ONLY(_##key = (on != 0)); <br>
> \<br>
> > ><br>
> > > NOT_JVMTI(report_unsupported(on)); <br>
> \<br>
> > ><br>
> > > } <br>
> \<br>
> > ><br>
> > > inline static bool key() { <br>
> \<br>
> > ><br>
> > > JVMTI_ONLY(return _##key); <br>
> \<br>
> > ><br>
> > > NOT_JVMTI(return false); <br>
> \<br>
> > ><br>
> > > }<br>
> > ><br>
> > > Should it (ie in a future bug/webrev)?<br>
> > ><br>
> > > Thanks,<br>
> > ><br>
> > > Jc<br>
> > ><br>
> ><br>
> ><br>
> <br>
> <br>
> <br>
> -- <br>
> <br>
> Thanks,<br>
> Jc<o:p></o:p></p>
</blockquote>
</div>
<p class="MsoNormal"><br clear="all">
<o:p></o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<p class="MsoNormal">-- <o:p></o:p></p>
<div>
<div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<p class="MsoNormal">Thanks, <o:p></o:p></p>
<div>
<p class="MsoNormal">Jc<o:p></o:p></p>
</div>
</div>
</div>
</div>
</div>
</div>
</body>
</html>