RFR (S) 8211980: Remove ThreadHeapSampler enable/disable/enabled methods
David Holmes
david.holmes at oracle.com
Thu Oct 11 22:42:23 UTC 2018
Thanks those updates look fine.
David
On 12/10/2018 8:37 AM, JC Beyler wrote:
> 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
> <http://cr.openjdk.java.net/%7Ejcbeyler/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
> <mailto: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>
> > <mailto: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>
> > <mailto: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>
> > <mailto: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>
> > <mailto: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>
> > <mailto: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>
> > <mailto: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/>
> > > >
> <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
More information about the serviceability-dev
mailing list