RFR (S) 8211980: Remove ThreadHeapSampler enable/disable/enabled methods

[JC Beyler]

That is my current belief and understanding. So looks like we are good to
go from a semantic point of view. Does anyone want to venture on a review
of the webrev?

Thanks!
Jc

On Thu, Oct 11, 2018 at 10:44 AM Hohensee, Paul <hohensee at amazon.com> 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?
>
> 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
>     >
>
>
>

-- 

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181011/908d4dda/attachment-0001.html>