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

[David Holmes]

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

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