Get rid of tlsTicker

RESOLVED FIXED in Firefox 54

Status

()

Core
Gecko Profiler
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: mstange, Assigned: njn)

Tracking

Trunk
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 affected, firefox54 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

6 months ago
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.
(Reporter)

Updated

6 months ago
Blocks: 1330184
(Assignee)

Comment 1

5 months ago
Created attachment 8830143 [details] [diff] [review]
(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.
Attachment #8830143 - Flags: review?(mstange)
(Assignee)

Updated

5 months ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 months ago
Created attachment 8830144 [details] [diff] [review]
(part 2) - Rename ProfileGatherer::mTicker as mSampler

It's a better name, given that the type is |GeckoSampler*|.
Attachment #8830144 - Flags: review?(mstange)
(Assignee)

Comment 3

5 months ago
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.
(Reporter)

Comment 4

5 months ago
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.
Attachment #8830143 - Flags: review?(mstange) → review+
(Reporter)

Comment 5

5 months ago
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.
Attachment #8830144 - Flags: review?(mstange) → review+
(Assignee)

Comment 6

5 months ago
Created attachment 8831023 [details] [diff] [review]
(part 3) - Remove GeckoSampler::sActiveSampler

I found that the GeckoSampler singleton is also being accessed via
sActiveSampler, and sometimes off-main-thread :(
Attachment #8831023 - Flags: review?(mstange)
(Reporter)

Updated

5 months ago
Attachment #8831023 - Flags: review?(mstange) → review+
(Assignee)

Comment 7

5 months ago
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

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e659096b1ca4
https://hg.mozilla.org/mozilla-central/rev/8b10b8a3dbde
https://hg.mozilla.org/mozilla-central/rev/fecc2109d459
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.