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

[JC Beyler]

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 <hohensee at 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-bounces at openjdk.java.net>
> on behalf of JC Beyler <jcbeyler at google.com>
> *Date: *Tuesday, October 9, 2018 at 11:58 PM
> *To: *"serviceability-dev at openjdk.java.net" <
> 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/
>
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181010/8cc0bc1e/attachment-0001.html>