<div dir="ltr"><div dir="ltr"><div dir="ltr">Hi Paul,<div><br></div><div>Thanks for the detailed answer.  I also moved everything into the class now to not have anything static outside anymore as you requested. </div><div><br></div><div>Webrev: <a href="http://cr.openjdk.java.net/~jcbeyler/8211980/webrev.01/">http://cr.openjdk.java.net/~jcbeyler/8211980/webrev.01/</a></div><div>Bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8211980">https://bugs.openjdk.java.net/browse/JDK-8211980</a></div><div><br></div><div>Just a few more notes on the webrev:</div><div> - I need to get that table initialized and I don't want to check if it is initialized during runtime, I want to "assume" it is in the normal code path; previously this was easy since I piggy-backed in the enable method to do it there</div><div> - With what you are saying in the ordering you are right but the only way I can do an initialization of an array via a method seems to be to at least set one static member of the class, I've put a boolean instead to be set to true</div><div><br></div><div>However, for the OrderAccess conversation, the conversation I had when I did this was that if you know it thread A can set a variable and thread B can get a variable, we should be using an OrderAccess and not assume that it "does not matter" that a thread gets a stale value. Because of that, I added them for ThreadHeapSampler but it seems that the JVMTI code I showed above does not do this. However, I can very well imagine a case where one thread calls SetEventNotification while another thread is doing some sort of work and trying to do get a value from one of those flags. Or perhaps there is an implicit ordering being done somewhere that I'm not aware of. Am I missing something?</div><div><br></div><div>Thanks,</div><div>Jc</div><div><br></div></div></div></div><br><div class="gmail_quote"><div dir="ltr">On Wed, Oct 10, 2018 at 10:10 AM Hohensee, Paul <<a href="mailto:hohensee@amazon.com" target="_blank">hohensee@amazon.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">





<div lang="EN-US" link="blue" vlink="purple">
<div class="m_3916631494531113978m_-3469260553783936175m_1164456280174544848WordSection1">
<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>”.<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></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().<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></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.<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></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.<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">Hope this helps,<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">Paul<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></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 <<a href="mailto:serviceability-dev-bounces@openjdk.java.net" target="_blank">serviceability-dev-bounces@openjdk.java.net</a>> on behalf of JC Beyler <<a href="mailto:jcbeyler@google.com" target="_blank">jcbeyler@google.com</a>><br>
<b>Date: </b>Tuesday, October 9, 2018 at 11:58 PM<br>
<b>To: </b>"<a href="mailto:serviceability-dev@openjdk.java.net" target="_blank">serviceability-dev@openjdk.java.net</a>" <<a href="mailto:serviceability-dev@openjdk.java.net" target="_blank">serviceability-dev@openjdk.java.net</a>><br>
<b>Subject: </b>RFR (S) 8211980: Remove ThreadHeapSampler enable/disable/enabled methods<u></u><u></u></span></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<div>
<div>
<div>
<div>
<p class="MsoNormal">Hi all,<br clear="all">
<u></u><u></u></p>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<div>
<div>
<p class="MsoNormal">When talking with Serguei about <a href="https://bugs.openjdk.java.net/browse/JDK-8201655" target="_blank">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.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Could I get a review for this:<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">Webrev: <a href="http://cr.openjdk.java.net/~jcbeyler/8211980/webrev.00/" target="_blank">http://cr.openjdk.java.net/~jcbeyler/8211980/webrev.00/</a><u></u><u></u></p>
</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><u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">This passed my testing on my dev machine in release and fastdebug.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></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:<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">  - When I did the enable/disable, I used OrderAccess to do so after a reviewer asked for it<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">  - From what I can tell, JVMTI_SUPPORT_FLAG does not use it and does instead:<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<div>
<p class="MsoNormal">#define JVMTI_SUPPORT_FLAG(key)                                           \<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">  private:                                                                \<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">  static bool  _##key;                                                    \<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">  public:                                                                 \<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">  inline static void set_##key(bool on) {                                 \<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">    JVMTI_ONLY(_##key = (on != 0));                                       \<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">    NOT_JVMTI(report_unsupported(on));                                    \<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">  }                                                                       \<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">  inline static bool key() {                                              \<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">    JVMTI_ONLY(return _##key);                                            \<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">    NOT_JVMTI(return false);                                              \<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">  }<u></u><u></u></p>
</div>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Should it (ie in a future bug/webrev)?<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<p class="MsoNormal">Thanks, <u></u><u></u></p>
<div>
<p class="MsoNormal">Jc<u></u><u></u></p>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>

</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="m_3916631494531113978m_-3469260553783936175gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><br></div>Thanks,<div>Jc</div></div></div>