Looks good to me. Paul
From: JC Beyler <jcbey...@google.com> Date: Thursday, October 11, 2018 at 6:38 PM To: David Holmes <david.hol...@oracle.com> Cc: "Hohensee, Paul" <hohen...@amazon.com>, "serviceability-dev@openjdk.java.net" <serviceability-dev@openjdk.java.net> Subject: Re: RFR (S) 8211980: Remove ThreadHeapSampler enable/disable/enabled methods Hi all, @David: I did your two requests for volatile and removing the lock The latest webrev is now: Webrev: http://cr.openjdk.java.net/~jcbeyler/8211980/webrev.02 Bug: https://bugs.openjdk.java.net/browse/JDK-8211980 Thanks, Jc On Thu, Oct 11, 2018 at 3:24 PM David Holmes <david.hol...@oracle.com<mailto:david.hol...@oracle.com>> wrote: On 12/10/2018 8:20 AM, JC Beyler wrote: > I'm 100% in agreement with David :) > > Inlined are my comments: > > On Thu, Oct 11, 2018 at 3:13 PM David Holmes > <david.hol...@oracle.com<mailto:david.hol...@oracle.com> > <mailto:david.hol...@oracle.com<mailto:david.hol...@oracle.com>>> wrote: > > On 12/10/2018 3:44 AM, Hohensee, Paul wrote: > > So, given that the lock was only used to protect log_table > initialization, and that the patch moves that into the C++ "class > initializer" which is run when the first Thread object is > constructed, we don't need any locking/memory ordering anymore, right? > > Right so: > > - ThreadHeapSampler_lock can be removed > - The load-acquire/release-store of _sampling_interval seem to serve no > purpose, but _sampling_interval should at least be marked volatile > (from > a documentation perspective if nothing else). If the intent was for a > change in sampling_interval to be immediately visible then a fence() > would be needed. > > > Right, I would leave them in place (or at least postpone the > conversation about them to a later webrev if someone feels really > strongly about them; I believe it does not hurt, this will never be > critical code but at least is semantically safe). Please deleted the unused ThreadHeapSampler_lock. Please mark _sampling_interval as volatile. > > The _log_table_initialized flag is not needed from the perspective > of an > initialization check - you can't run code until after the static > initializers have run (though I'm unclear how C++ manages this from a > concurrency perspective). But the flag may be needed just as a means to > separate the allocation of the table from the initialization of it - > again I'm unclear how C++ static initialization works in detail (you > have to be very careful with such initialization to ensure there are > zero dependencies on anything done as part of VM initialization). > > > Exactly. I cannot force the initialization of a static array via a > method without initialization *something*. It can be to initialize a > pointer to the array and I just use the pointer in the class; that was > what the first webrev one was doing. Using a boolean such as here seems > to be the lesser evil and allows us to add an assert that all is well in > the world at the only usage point of the array in assert mode. > > So I'm happy with the current form (I'm biased since I sent the webrev > for review :-)), any LGTM or other comments? I can't comment on the details of the code just the general structural changes, and they seem okay to me. Thanks, David > Jc > > David > > > Paul > > > > On 10/11/18, 4:11 AM, "David Holmes" > <david.hol...@oracle.com<mailto:david.hol...@oracle.com> > <mailto:david.hol...@oracle.com<mailto:david.hol...@oracle.com>>> wrote: > > > > On 11/10/2018 3:10 AM, Hohensee, Paul wrote: > > > 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>”. > > > > > > 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. > > > > The release_store and load_acquire were necessary to ensure the > > lock-free enabled() check ensured visibility of the > initialization of > > the data structures in the ensuing code. Otherwise you'd > need to grab > > the lock on the enabled() checks, which is too heavy-weight. > The lock is > > only used to ensure single-threaded initialization of the > log_table, > > actual accesses are again lock-free. > > > > David > > > > 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(). > > > > > > 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. > > > > > > 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. > > > > > > Hope this helps, > > > > > > Paul > > > > > > *From: *serviceability-dev > > <serviceability-dev-boun...@openjdk.java.net<mailto:serviceability-dev-boun...@openjdk.java.net> > > <mailto:serviceability-dev-boun...@openjdk.java.net<mailto:serviceability-dev-boun...@openjdk.java.net>>> > > > on behalf of JC Beyler > <jcbey...@google.com<mailto:jcbey...@google.com> > <mailto:jcbey...@google.com<mailto:jcbey...@google.com>>> > > > *Date: *Tuesday, October 9, 2018 at 11:58 PM > > > *To: > *"serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.java.net> > > <mailto:serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.java.net>>" > > > > <serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.java.net> > > <mailto:serviceability-dev@openjdk.java.net<mailto:serviceability-dev@openjdk.java.net>>> > > > *Subject: *RFR (S) 8211980: Remove ThreadHeapSampler > > > enable/disable/enabled methods > > > > > > Hi all, > > > > > > When talking with Serguei about JDK-8201655 > > > <https://bugs.openjdk.java.net/browse/JDK-8201655>, we > talked about why > > > ThreadHeapSampler has an enabled/disabled when we could > have just used > > > the should_post_sampled_object_alloc to begin with. > > > > > > Could I get a review for this: > > > > > > Webrev: > http://cr.openjdk.java.net/~jcbeyler/8211980/webrev.00/ > <http://cr.openjdk.java.net/%7Ejcbeyler/8211980/webrev.00/> > > > <http://cr.openjdk.java.net/%7Ejcbeyler/8211980/webrev.00/> > > > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8211980 > > > > > > This passed my testing on my dev machine in release and > fastdebug. > > > > > > 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: > > > > > > - When I did the enable/disable, I used OrderAccess to > do so after a > > > reviewer asked for it > > > > > > - From what I can tell, JVMTI_SUPPORT_FLAG does not use > it and does > > > instead: > > > > > > #define JVMTI_SUPPORT_FLAG(key) > \ > > > > > > private: > \ > > > > > > static bool _##key; > \ > > > > > > public: > \ > > > > > > inline static void set_##key(bool on) { > \ > > > > > > JVMTI_ONLY(_##key = (on != 0)); > \ > > > > > > NOT_JVMTI(report_unsupported(on)); > \ > > > > > > } > \ > > > > > > inline static bool key() { > \ > > > > > > JVMTI_ONLY(return _##key); > \ > > > > > > NOT_JVMTI(return false); > \ > > > > > > } > > > > > > Should it (ie in a future bug/webrev)? > > > > > > Thanks, > > > > > > Jc > > > > > > > > > > > -- > > Thanks, > Jc -- Thanks, Jc
