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

JC Beyler jcbeyler at google.com
Thu Oct 11 22:37:18 UTC 2018


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.holmes at 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.holmes at oracle.com
> > <mailto:david.holmes at 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.holmes at oracle.com
> >     <mailto: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
> >     <mailto:serviceability-dev-bounces at openjdk.java.net>>
> >      >      > on behalf of JC Beyler <jcbeyler at google.com
> >     <mailto:jcbeyler at google.com>>
> >      >      > *Date: *Tuesday, October 9, 2018 at 11:58 PM
> >      >      > *To: *"serviceability-dev at openjdk.java.net
> >     <mailto:serviceability-dev at openjdk.java.net>"
> >      >      > <serviceability-dev at openjdk.java.net
> >     <mailto: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/>
> >      >      > <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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181011/f15c38b7/attachment-0001.html>


More information about the serviceability-dev mailing list