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