1328365 - Get rid of tlsTicker

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Markus Stange [:mstange]]

From bug 1135236 comment 15:

> 1. Get rid of tlsTicker.
>   - all accesses to tlsTicker currently seem to be happening from only main thread.
>   - there should only ever be one instance of TableTicker anyway.

Comment 1

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 1) - Replace tlsTicker with gSampler

There is a single GeckoSampler and it is currently only accessed on the main
thread, so it's silly to use TLS for it; a normal global variable is better.

This patch also adds main thread assertions to a number of the profiler_*()
functions. Even though bug 1330184 may get rid of some of them, right now they
are a useful as both a sanity check and documentation.

Comment 2

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 2) - Rename ProfileGatherer::mTicker as mSampler

It's a better name, given that the type is |GeckoSampler*|.

Comment 3

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Interesting... I did a try run and the NS_IsMainThread() assertion I added to PseudoStack::flushSamplerOnJSShutdown() in part 1 is failing, presumably because it's being called on other threads. And in those cases it must be a no-op because the current tlsTicker.get() must fail. I'm not sure what effect this has.

Comment 4

[Markus Stange [:mstange]]

Comment on attachment 8830143 [details] [diff] [review]
(part 1) - Replace tlsTicker with gSampler

Review of attachment 8830143 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/core/GeckoSampler.cpp
@@ -607,3 @@
>    MOZ_ASSERT(mContext);
> -  GeckoSampler* t = tlsTicker.get();
> -  if (t) {

Heh. So it looks like this never worked for DOM Worker threads. (http://searchfox.org/mozilla-central/rev/bf98cd4315b5efa1b28831001ad27d54df7bbb68/dom/workers/RuntimeService.cpp#2895 definitely doesn't run on the main thread, and it calls sampleContext with a null context, which calls flushSamplerOnJSShutdown.)
And this is just one of many issues around profiling DOM Workers...

I think for now we should just turn the release assert into an early exit so that we keep the existing behavior and can land this patch.

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 8830144 [details] [diff] [review]
(part 2) - Rename ProfileGatherer::mTicker as mSampler

Review of attachment 8830144 [details] [diff] [review]:
-----------------------------------------------------------------

After this patch, there is one last reference to the GeckoSampler as "TableTicker" in js/src/vm/GeckoProfiler.cpp.

Comment 6

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 3) - Remove GeckoSampler::sActiveSampler

I found that the GeckoSampler singleton is also being accessed via
sActiveSampler, and sometimes off-main-thread :(

Comment 7

[Nicholas Nethercote [:njn] (On Long Service Leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e659096b1ca44072fa11e555538320a13ff56b07
Bug 1328365 (part 1) - Replace tlsTicker with gSampler. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b10b8a3dbde62240b4cc53c972a91236871845c
Bug 1328365 (part 2) - Rename ProfileGatherer::mTicker as mSampler. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fecc2109d459d98f68cccb3ebf395f048d0f9e5e
Bug 1328365 (part 3) - Remove GeckoSampler::sActiveSampler. r=mstange.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e659096b1ca4
https://hg.mozilla.org/mozilla-central/rev/8b10b8a3dbde
https://hg.mozilla.org/mozilla-central/rev/fecc2109d459