Once tlsTicker is removed and we can access the GeckoSampler on any thread (bug 1328365), we should make it possible to control the profiler and capture profiles on any thread. This will help in getting profiles from the child process if the child process is hanging.

Bug 1328365 has been resolved, but most Sampler accesses are still from the main thread and have assertions ensuring this. I have some major concerns about thread safety already in the profiler, and I am working on understanding things enough to fix them. I'd like to do that first before increasing the amount of off-main-thread work! :)

mstange, is just allowing profiler_save_profile_to_file_async() to be called off-main-thread enough?

For bug 1330185 we're going to need all of profiler_start, _stop, _pause, _resume, and _get_profile. Also probably _lock and _unlock if we're going to fix that.

Note: the way notifications were handled in bug 1346356 is not safe when profiler_start() and profiler_stop() can be called from off the main thread -- it's possible that the notifications might be received in the wrong order. That is fixable with some effort.

I've attached a patch to bug 1354961 that collect the profile on a background thread, so that we can test-run a world in which these functions are not called on the main thread.

Comment on attachment 8868319 [details] Bug 1330184 - Allow StreamMetaObject to be called on a background thread, but only include startTime and version for those calls. https://reviewboard.mozilla.org/r/139902/#review143290 ::: tools/profiler/core/platform.cpp:1392 (Diff revision 1) > + if (!NS_IsMainThread()) { > + // Leave the rest of the properties out if we're not on the main thread. > + // At the moment, the only case in which this function is called on a > + // background thread is if we're in a content process and are going to > + // send this profile to the parent process. In that case, the parent > + // process profile's "meta" object already has all this information, and I'd replace "all this information" with "the rest of the properties", to tie it more obviously back to the earlier part of the sentence.

Comment on attachment 8868320 [details] Bug 1330184 - Register/unregister the IOInterposeObserver on the main thread, regardless of what thread the profiler is started / stopped on. https://reviewboard.mozilla.org/r/139904/#review143292 ::: tools/profiler/core/platform.cpp:313 (Diff revision 1) > + IOInterposer::Register(IOInterposeObserver::OpAll, mInterposeObserver); > + } else { > + RefPtr<ProfilerIOInterposeObserver> observer = mInterposeObserver; > + NS_DispatchToMainThread(NS_NewRunnableFunction([=]() { > + IOInterposer::Register(IOInterposeObserver::OpAll, observer); > + })); It feels like this might be a common pattern. Shame that we have to duplicate code.

Comment on attachment 8868321 [details] Bug 1330184 - Remove main-thread-only assertions. https://reviewboard.mozilla.org/r/139906/#review143316 A bunch of functions in platform.cpp and platform-linux-android.cpp have a comment saying "This function runs off the main thread" or similar (grep for "This function"). These comments made sense when such functions were in the minority, but now main-thread-only functions are in the minority. So I think all those comments can now be removed. As a replacement it might be worth adding a comment at the top of the file saying something like "All functions can run on multiple threads unless they have an NS_IsMainThread() assertion."

Pushed by mstange@themasta.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8c96ac2d762f Allow notifying observers for profiler state changes on background threads. r=njn https://hg.mozilla.org/integration/mozilla-inbound/rev/edfdb14bd020 Allow StreamMetaObject to be called on a background thread, but only include startTime and version for those calls. r=njn https://hg.mozilla.org/integration/mozilla-inbound/rev/fd537bffda4b Register/unregister the IOInterposeObserver on the main thread, regardless of what thread the profiler is started / stopped on. r=njn https://hg.mozilla.org/integration/mozilla-inbound/rev/327c145ded03 Remove main-thread-only assertions. r=njn

Backout by mstange@themasta.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/61095bd2cbca Back out and bug 1330185 because of test failures.

Pushed by mstange@themasta.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/52489c7eadaf Allow notifying observers for profiler state changes on background threads. r=njn https://hg.mozilla.org/integration/mozilla-inbound/rev/e13b9e798e16 Allow StreamMetaObject to be called on a background thread, but only include startTime and version for those calls. r=njn https://hg.mozilla.org/integration/mozilla-inbound/rev/dbe452a9eebb Register/unregister the IOInterposeObserver on the main thread, regardless of what thread the profiler is started / stopped on. r=njn https://hg.mozilla.org/integration/mozilla-inbound/rev/2b8d50fcb20f Remove main-thread-only assertions. r=njn

Looks like you didn't remove the comments I mentioned in comment 6?

Oops, you're right, I forgot to address all your comments in this bug. Sorry! I'll take care of them in a new bug tomorrow.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/df065d529e85237953033deaf3bd17e4281baa9f for Win8 leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=101099619&repo=mozilla-inbound

(In reply to Nicholas Nethercote [:njn] from comment #4) > Note: the way notifications were handled in bug 1346356 is not safe when > profiler_start() and profiler_stop() can be called from off the main thread > -- it's possible that the notifications might be received in the wrong > order. That is fixable with some effort. mstange, will the profiler add-on handle out-of-order notifications gracefully?

(In reply to Nicholas Nethercote [:njn] from comment #18) > (In reply to Nicholas Nethercote [:njn] from comment #4) > > Note: the way notifications were handled in bug 1346356 is not safe when > > profiler_start() and profiler_stop() can be called from off the main thread > > -- it's possible that the notifications might be received in the wrong > > order. That is fixable with some effort. > > mstange, will the profiler add-on handle out-of-order notifications > gracefully? No it will not. But it only listens for them in the parent process, and at the moment, in the parent process it's only the main thread that changes the profiler state. So for now we can get away with it. I realized that I hadn't requested review from you on the first patch in this bug's series (the NotifyObservers one). I've made some modifications to it and added a comment.

Comment on attachment 8872471 [details] Bug 1330184 - Allow notifying observers for profiler state changes on background threads. https://reviewboard.mozilla.org/r/143998/#review147706

One thing this bug lacks is tests. Whether it's this bug or a follow-up, we should augment the gtest with some multi-threaded stop/start action.

Unfortunately the always-async observer notifications caused a test failure in a devtools profiler test: http://searchfox.org/mozilla-central/rev/31eec6f59eb303adf3318058cb30bd6a883115a5/devtools/server/tests/unit/test_profiler_events-01.js#42-46 This test expects "profiler-started" and "profiler-stopped" events to have fired by the time the .startProfiler() / .stopProfiler() promise resolves. That's not the case anymore with the previous patch. I don't think the actual devtool makes this assumption; it doesn't seem to listen for "profiler-started" at all and reacts to "profiler-stopped" simply by logging an error: http://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/devtools/server/performance/recorder.js#159 However, I didn't know how to change the test, and I'm not 100% sure about the devtools code, so I just went back to my previous implementation that fires the notifications synchronously when called on the main thread. I also added a few small tests. The "threadsafe refcounting" patch is pulled out from bug 1330185 because it's necessary to get the tests passing.

Comment on attachment 8872790 [details] Bug 1330184 - Use threadsafe refcounting for nsProfilerStartParams. https://reviewboard.mozilla.org/r/144292/#review148112 r=me, but it would be nice to have a brief comment above the NS_DECL_THREADSAFE_ISUPPORTS explaining why threadsafe refcounting is required.

Comment on attachment 8872789 [details] Bug 1330184 - Add some GeckoProfiler tests that call functions on a background thread. https://reviewboard.mozilla.org/r/144290/#review148118 Thank you for adding these. The use of NS_DISPATCH_SYNC means that there is never any contention over the shared data structures. I might do a follow-up that introduces some genuine contention.

Pushed by mstange@themasta.com: https://hg.mozilla.org/integration/autoland/rev/b30c003db0c8 Allow notifying observers for profiler state changes on background threads. r=njn https://hg.mozilla.org/integration/autoland/rev/4f297a3633aa Allow StreamMetaObject to be called on a background thread, but only include startTime and version for those calls. r=njn https://hg.mozilla.org/integration/autoland/rev/15728eea1798 Register/unregister the IOInterposeObserver on the main thread, regardless of what thread the profiler is started / stopped on. r=njn https://hg.mozilla.org/integration/autoland/rev/640a79142377 Use threadsafe refcounting for nsProfilerStartParams. r=njn https://hg.mozilla.org/integration/autoland/rev/ac273d41a044 Remove main-thread-only assertions. r=njn https://hg.mozilla.org/integration/autoland/rev/c9669890cf99 Add some GeckoProfiler tests that call functions on a background thread. r=njn

https://hg.mozilla.org/mozilla-central/rev/b30c003db0c8 https://hg.mozilla.org/mozilla-central/rev/4f297a3633aa https://hg.mozilla.org/mozilla-central/rev/15728eea1798 https://hg.mozilla.org/mozilla-central/rev/640a79142377 https://hg.mozilla.org/mozilla-central/rev/ac273d41a044 https://hg.mozilla.org/mozilla-central/rev/c9669890cf99

Wontfixing for 54 since we're a week away from release.