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.hol...@oracle.com
<mailto:david.hol...@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.hol...@oracle.com <mailto:david.hol...@oracle.com>
> <mailto:david.hol...@oracle.com
<mailto:david.hol...@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.hol...@oracle.com <mailto:david.hol...@oracle.com>
> <mailto:david.hol...@oracle.com
<mailto:david.hol...@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-boun...@openjdk.java.net
<mailto:serviceability-dev-boun...@openjdk.java.net>
> <mailto:serviceability-dev-boun...@openjdk.java.net
<mailto:serviceability-dev-boun...@openjdk.java.net>>>
> > > on behalf of JC Beyler <jcbey...@google.com
<mailto:jcbey...@google.com>
> <mailto:jcbey...@google.com <mailto:jcbey...@google.com>>>
> > > *Date: *Tuesday, October 9, 2018 at 11:58 PM
> > > *To: *"serviceability-dev@openjdk.java.net
<mailto:serviceability-dev@openjdk.java.net>
> <mailto:serviceability-dev@openjdk.java.net
<mailto:serviceability-dev@openjdk.java.net>>"
> > > <serviceability-dev@openjdk.java.net
<mailto:serviceability-dev@openjdk.java.net>
> <mailto:serviceability-dev@openjdk.java.net
<mailto:serviceability-dev@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
