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

Discussion:

JC Beyler

2018-10-10 03:57:45 UTC

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/
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

Hohensee, Paul

2018-10-10 17:10:29 UTC

Permalink

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

JC Beyler

2018-10-10 20:19:34 UTC

Permalink

Hi Paul,

Thanks for the detailed answer. I also moved everything into the class now
to not have anything static outside anymore as you requested.

Webrev: http://cr.openjdk.java.net/~jcbeyler/8211980/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8211980

Just a few more notes on the webrev:
- I need to get that table initialized and I don't want to check if it is
initialized during runtime, I want to "assume" it is in the normal code
path; previously this was easy since I piggy-backed in the enable method to
do it there
- With what you are saying in the ordering you are right but the only way
I can do an initialization of an array via a method seems to be to at least
set one static member of the class, I've put a boolean instead to be set to
true

However, for the OrderAccess conversation, the conversation I had when I
did this was that if you know it thread A can set a variable and thread B
can get a variable, we should be using an OrderAccess and not assume that
it "does not matter" that a thread gets a stale value. Because of that, I
added them for ThreadHeapSampler but it seems that the JVMTI code I showed
above does not do this. However, I can very well imagine a case where one
thread calls SetEventNotification while another thread is doing some sort
of work and trying to do get a value from one of those flags. Or perhaps
there is an implicit ordering being done somewhere that I'm not aware of.
Am I missing something?

Thanks,
Jc

Post by Hohensee, Paul
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. 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
*Date: *Tuesday, October 9, 2018 at 11:58 PM
*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.
Webrev: http://cr.openjdk.java.net/~jcbeyler/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
- When I did the enable/disable, I used OrderAccess to do so after a
reviewer asked for it
#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

Hohensee, Paul

2018-10-11 20:00:40 UTC

Permalink

I donât think youâre missing anything, see my response to David. I donât think you need _log_table_initialized, but Iâll defer to David.

Paul

From: JC Beyler <***@google.com>
Date: Wednesday, October 10, 2018 at 4:20 PM
To: "Hohensee, Paul" <***@amazon.com>
Cc: "serviceability-***@openjdk.java.net" <serviceability-***@openjdk.java.net>
Subject: Re: RFR (S) 8211980: Remove ThreadHeapSampler enable/disable/enabled methods

Hi Paul,

Thanks for the detailed answer. I also moved everything into the class now to not have anything static outside anymore as you requested.

Webrev: http://cr.openjdk.java.net/~jcbeyler/8211980/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8211980

Just a few more notes on the webrev:
- I need to get that table initialized and I don't want to check if it is initialized during runtime, I want to "assume" it is in the normal code path; previously this was easy since I piggy-backed in the enable method to do it there
- With what you are saying in the ordering you are right but the only way I can do an initialization of an array via a method seems to be to at least set one static member of the class, I've put a boolean instead to be set to true

However, for the OrderAccess conversation, the conversation I had when I did this was that if you know it thread A can set a variable and thread B can get a variable, we should be using an OrderAccess and not assume that it "does not matter" that a thread gets a stale value. Because of that, I added them for ThreadHeapSampler but it seems that the JVMTI code I showed above does not do this. However, I can very well imagine a case where one thread calls SetEventNotification while another thread is doing some sort of work and trying to do get a value from one of those flags. Or perhaps there is an implicit ordering being done somewhere that I'm not aware of. Am I missing something?

Thanks,
Jc

On Wed, Oct 10, 2018 at 10:10 AM Hohensee, Paul <***@amazon.com<mailto:***@amazon.com>> 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. 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-***@openjdk.java.net<mailto:serviceability-dev-***@openjdk.java.net>> on behalf of JC Beyler <***@google.com<mailto:***@google.com>>
Date: Tuesday, October 9, 2018 at 11:58 PM
To: "serviceability-***@openjdk.java.net<mailto:serviceability-***@openjdk.java.net>" <serviceability-***@openjdk.java.net<mailto:serviceability-***@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/
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

David Holmes

2018-10-10 23:22:07 UTC

Permalink

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>”.

Namespaces are used in a number of places now. There is still a
prohibition on "using namespace ...".

David

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. 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
*Date: *Tuesday, October 9, 2018 at 11:58 PM
*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.
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
- 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
#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

David Holmes

2018-10-11 08:10:20 UTC

Permalink

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
*Date: *Tuesday, October 9, 2018 at 11:58 PM
*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.
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
- 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
#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

Hohensee, Paul

2018-10-11 17:44:30 UTC

Permalink

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?

Paul

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
*Date: *Tuesday, October 9, 2018 at 11:58 PM
*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.
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
- When I did the enable/disable, I used OrderAccess to do so after a
reviewer asked for it
#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)?

JC Beyler

2018-10-11 19:49:43 UTC

Permalink

That is my current belief and understanding. So looks like we are good to
go from a semantic point of view. Does anyone want to venture on a review
of the webrev?

Thanks!
Jc

Post by Hohensee, Paul
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?
Paul

Post by Hohensee, Paul
There used to be a rule against using ânamespaceâ. Donât know if

thatâs

Post by Hohensee, Paul
still true or not: I believe it is. Hence the âstatic const

<whatever>â.

Post by Hohensee, Paul
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

Post by Hohensee, Paul
that log_table was initialized and its content available to other
threads before the _enabled flag was set. I.e., the _enabled flag

acted

Post by Hohensee, Paul
as a store visibility barrier for log_table, and possibly other
ThreadHeapSampler data structures. If no thread can ever see a

partially

Post by Hohensee, Paul
initialized log_table, then no need for ordering. The old code used

Post by Hohensee, Paul
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()

Post by Hohensee, Paul
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

Post by Hohensee, Paul
was some reason to make sure all threads saw previous stores to
ThreadHeapSampler related memory before _enabled was set to zero.

The

Post by Hohensee, Paul
load_acquire in enabled() may not have been needed either, because

Post by Hohensee, Paul
only prevents subsequent loads from being executed before the load

from

Post by Hohensee, Paul
_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

Post by Hohensee, Paul
_enabled was set to one and seen by enabled().
Yeah, thatâs hard to follow, and I wrote it. :) It comes down to

what

Post by Hohensee, Paul
youâre guarding with OrderAccess. If itâs only ThreadHeapSampler

data,

Post by Hohensee, Paul
and since only a Thread has one, and since ThreadHeapSampler statics

are

Post by Hohensee, Paul
initialized before construction of the first _heap_sampler, and

since

Post by Hohensee, Paul
the construction of a Thread is guarded by multiple mutexes which

will

Post by Hohensee, Paul
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

Post by Hohensee, Paul
rather than define them at file scope in threadHeapSampler.cpp.

I.e.,

Post by Hohensee, Paul
all of FastLogNumBits, FastLogMask, and internal_log_table (and name

Post by Hohensee, Paul
back to that log_table). File scope data is a no-no.
Hope this helps,
Paul
*From: *serviceability-dev <
*Date: *Tuesday, October 9, 2018 at 11:58 PM
*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

Post by Hohensee, Paul
ThreadHeapSampler has an enabled/disabled when we could have just

used

Post by Hohensee, Paul
the should_post_sampled_object_alloc to begin with.
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

Post by Hohensee, Paul
reduce email spam and because it should be included in the review I
- When I did the enable/disable, I used OrderAccess to do so

after a

Post by Hohensee, Paul
reviewer asked for it
- From what I can tell, JVMTI_SUPPORT_FLAG does not use it and

does

Post by Hohensee, Paul
#define JVMTI_SUPPORT_FLAG(key)

\
\

Post by Hohensee, Paul
static bool _##key;

\
\

Post by Hohensee, Paul
inline static void set_##key(bool on) {

Post by Hohensee, Paul
JVMTI_ONLY(_##key = (on != 0));

Post by Hohensee, Paul
NOT_JVMTI(report_unsupported(on));

Post by Hohensee, Paul
}

Post by Hohensee, Paul
inline static bool key() {

Post by Hohensee, Paul
JVMTI_ONLY(return _##key);

Post by Hohensee, Paul
NOT_JVMTI(return false);

Post by Hohensee, Paul
}
Should it (ie in a future bug/webrev)?
Thanks,
Jc

--
Thanks,
Jc

David Holmes

2018-10-11 22:13:45 UTC

Permalink

Post by Hohensee, Paul
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.

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).

David

Post by Hohensee, Paul
Paul

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

JC Beyler

2018-10-11 22:20:17 UTC

Permalink

I'm 100% in agreement with David :)