RFR (S) 8211980: Remove ThreadHeapSampler enable/disable/enabled methods
David Holmes
david.holmes at oracle.com
Thu Oct 11 22:13:45 UTC 2018
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.
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).
David
> Paul
>
> On 10/11/18, 4:11 AM, "David Holmes" <david.holmes at 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-bounces at openjdk.java.net>
> > on behalf of JC Beyler <jcbeyler at google.com>
> > *Date: *Tuesday, October 9, 2018 at 11:58 PM
> > *To: *"serviceability-dev at openjdk.java.net"
> > <serviceability-dev at 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/>
> >
> > 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
> >
>
>
More information about the serviceability-dev
mailing list