Closed Bug 1330184 Opened 7 years ago Closed 7 years ago

Allow profiler_* functions to be called off the main thread

Categories

(Core :: Gecko Profiler, defect)

Product:
Core
Component:
Gecko Profiler
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(6 files)

Bug 1330184 - Allow StreamMetaObject to be called on a background thread, but only include startTime and version for those calls.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1330184 - Register/unregister the IOInterposeObserver on the main thread, regardless of what thread the profiler is started / stopped on.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1330184 - Remove main-thread-only assertions.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1330184 - Allow notifying observers for profiler state changes on background threads.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1330184 - Add some GeckoProfiler tests that call functions on a background thread.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1330184 - Use threadsafe refcounting for nsProfilerStartParams.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details 
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.
Blocks: 1330185
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?
Flags: needinfo?(mstange)
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.
Flags: needinfo?(mstange)
Depends on: 1342306
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.
Blocks: 1354961
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.
Attachment #8868319 - Flags: review?(n.nethercote) → review+
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.
Attachment #8868320 - Flags: review?(n.nethercote) → review+
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."
Attachment #8868321 - Flags: review?(n.nethercote) → review+
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
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.
(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?
Flags: needinfo?(mstange)
(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.
Flags: needinfo?(mstange)
Comment on attachment 8872471 [details]
Bug 1330184 - Allow notifying observers for profiler state changes on background threads.

https://reviewboard.mozilla.org/r/143998/#review147706
Attachment #8872471 - Flags: review?(n.nethercote) → review+
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.
Attachment #8872790 - Flags: review?(n.nethercote) → review+
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.
Attachment #8872789 - Flags: review?(n.nethercote) → review+
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
Wontfixing for 54 since we're a week away from release.
status-firefox53: affected → wontfix
status-firefox54: --- → wontfix
Assignee: nobody → mstange
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: