Get rid of tlsTicker

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
2 years 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

2 years 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

2 years ago
Blocks: 1330184
(Assignee)

Comment 1

2 years ago
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

2 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 2

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

Comment 3

2 years 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

2 years 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

2 years 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

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

Updated

2 years ago
Attachment #8831023 - Flags: review?(mstange) → review+

Comment 8

2 years 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: 2 years 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.