<div dir="ltr">Hi all,<div><br></div><div>@David: I did your two requests for volatile and removing the lock</div><div><br></div><div>The latest webrev is now:</div><div>Webrev: <a href="http://cr.openjdk.java.net/~jcbeyler/8211980/webrev.02" target="_blank">http://cr.openjdk.java.net/~jcbeyler/8211980/webrev.02</a><br></div><div><div>Bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8211980" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8211980</a></div><br class="gmail-Apple-interchange-newline"><div class="gmail_quote"><div>Thanks,</div><div>Jc</div><div><br></div><div dir="ltr">On Thu, Oct 11, 2018 at 3:24 PM David Holmes <<a href="mailto:david.holmes@oracle.com">david.holmes@oracle.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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.<br>
<br></blockquote><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> <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" rel="noreferrer" 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/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jcbeyler/8211980/webrev.00/</a><br>
>     <<a href="http://cr.openjdk.java.net/%7Ejcbeyler/8211980/webrev.00/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8211980/webrev.00/</a>><br>
>      >      > <<a href="http://cr.openjdk.java.net/%7Ejcbeyler/8211980/webrev.00/" rel="noreferrer" 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" rel="noreferrer" 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<br>
</blockquote></div><br clear="all"><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></div></div>