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>> 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>> 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>>
> > on behalf of JC Beyler <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>"
> > <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/>
> >
> > 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
