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

Hohensee, Paul hohensee at amazon.com
Thu Oct 11 20:00:40 UTC 2018 

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 <jcbeyler at google.com>
Date: Wednesday, October 10, 2018 at 4:20 PM
To: "Hohensee, Paul" <hohensee at amazon.com>
Cc: "serviceability-dev at openjdk.java.net" <serviceability-dev at 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 <hohensee at amazon.com<mailto: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<mailto:serviceability-dev-bounces at openjdk.java.net>> on behalf of JC Beyler <jcbeyler at google.com<mailto:jcbeyler at google.com>>
Date: Tuesday, October 9, 2018 at 11:58 PM
To: "serviceability-dev at openjdk.java.net<mailto:serviceability-dev at openjdk.java.net>" <serviceability-dev at openjdk.java.net<mailto: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/20181011/012c4bf6/attachment-0001.html>

More information about the serviceability-dev mailing list