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