<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">There used to be a rule against using “namespace”. Don’t know if that’s still true or not: I believe it is. Hence the “static const <whatever>”.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">You only need OrderAccess if there could be a race on accesses to whatever you’re guarding. Looks like the old code wanted to make sure that log_table was initialized and its content available to other threads before the _enabled flag was
 set. I.e., the _enabled flag acted as a store visibility barrier for log_table, and possibly other ThreadHeapSampler data structures. If no thread can ever see a partially initialized log_table, then no need for ordering. The old code used a MutexLocker in
 init_log_table(), which former has a fence in its destructor and probably (haven’t dived into the code) guards all accesses to log_table, so the release_store() on _enabled in enable() was redundant. Same with the release_store() in disable(), unless there
 was some reason to make sure all threads saw previous stores to ThreadHeapSampler related memory before _enabled was set to zero. The load_acquire in enabled() may not have been needed either, because it only prevents subsequent loads from being executed before
 the load from _enabled, so if _enabled was being used to guard access only to ThreadHeapSampler data such as log_table, the release_store() on _enabled would guarantee that all necessary stores would be done before _enabled was set to one and seen by enabled().<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Yeah, that’s hard to follow, and I wrote it. :) It comes down to what you’re guarding with OrderAccess. If it’s only ThreadHeapSampler data, and since only a Thread has one, and since ThreadHeapSampler statics are initialized before construction
 of the first _heap_sampler, and since the construction of a Thread is guarded by multiple mutexes which will force visibility of any ThreadHeapSampler statics before a Thread is used, you don’t need OrderAccess.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">I’d put anything to do with ThreadHeapSampler into its class definition rather than define them at file scope in threadHeapSampler.cpp. I.e., all of FastLogNumBits, FastLogMask, and internal_log_table (and name it back to that log_table).
 File scope data is a no-no.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Hope this helps,<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">serviceability-dev <serviceability-dev-bounces@openjdk.java.net> on behalf of JC Beyler <jcbeyler@google.com><br>
<b>Date: </b>Tuesday, October 9, 2018 at 11:58 PM<br>
<b>To: </b>"serviceability-dev@openjdk.java.net" <serviceability-dev@openjdk.java.net><br>
<b>Subject: </b>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>
<div>
<div>
<div>
<div>
<p class="MsoNormal">Hi all,<br clear="all">
<o:p></o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<div>
<p class="MsoNormal">When talking with Serguei about <a href="https://bugs.openjdk.java.net/browse/JDK-8201655">JDK-8201655</a>, we talked about why ThreadHeapSampler has an enabled/disabled when we could have just used the should_post_sampled_object_alloc
 to begin with.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Could I get a review for this:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">Webrev: <a href="http://cr.openjdk.java.net/~jcbeyler/8211980/webrev.00/">http://cr.openjdk.java.net/~jcbeyler/8211980/webrev.00/</a><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">Bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8211980">https://bugs.openjdk.java.net/browse/JDK-8211980</a><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">This passed my testing on my dev machine in release and fastdebug.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">The question I would like to raise here at the same time (in order to reduce email spam and because it should be included in the review I believe) is:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">  - When I did the enable/disable, I used OrderAccess to do so after a reviewer asked for it<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">  - From what I can tell, JVMTI_SUPPORT_FLAG does not use it and does instead:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<div>
<p class="MsoNormal">#define JVMTI_SUPPORT_FLAG(key)                                           \<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">  private:                                                                \<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">  static bool  _##key;                                                    \<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">  public:                                                                 \<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">  inline static void set_##key(bool on) {                                 \<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">    JVMTI_ONLY(_##key = (on != 0));                                       \<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">    NOT_JVMTI(report_unsupported(on));                                    \<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">  }                                                                       \<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">  inline static bool key() {                                              \<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">    JVMTI_ONLY(return _##key);                                            \<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">    NOT_JVMTI(return false);                                              \<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">  }<o:p></o:p></p>
</div>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Should it (ie in a future bug/webrev)?<o:p></o:p></p>
</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>
</div>
</div>
</div>
</body>
</html>