Get rid of tlsTicker

RESOLVED FIXED in Firefox 54



2 years ago
2 years ago


(Reporter: mstange, Assigned: njn)


Dependency tree / graph

Firefox Tracking Flags

(firefox53 affected, firefox54 fixed)



(3 attachments)



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.


2 years ago
Blocks: 1330184

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)


2 years ago
Assignee: nobody → n.nethercote

Comment 2

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

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.

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. ( 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+

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+

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)


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

Comment 8

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