1329684 - remove ::Mutex/::MutexAutoLock wrappers

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Nathan Froyd [:froydnj]]

These are unnecessary, and incur virtual calls during locking (which may be able to be elided by sufficiently smart link-time optimization).

We have two options for removing these:

1) Use Gecko's Mutex everywhere.
2) Use std::mutex everywhere.

Option 2 would make SPS_STANDALONE builds somewhat easier, though I note that:

http://searchfox.org/mozilla-central/rev/8144fcc62054e278fe5db9666815cc13188c96fa/tools/profiler/core/platform.cpp#1313
http://searchfox.org/mozilla-central/rev/8144fcc62054e278fe5db9666815cc13188c96fa/tools/profiler/core/platform.cpp#1343

i.e. SPS_STANDALONE builds are already busted right now, as they're both using GeckoMutex.  Note also that the documentation:

http://searchfox.org/mozilla-central/source/tools/profiler/core/platform.h#127

says that mutexes should support recursive locking by default (ugh), which std::mutex doesn't support.  Ideally we're not using recursive locking anywhere, but I'm sure that if we went with std::mutex, we would find places that did...

Comment 1

[Markus Stange [:mstange]]

How about attacking this in the following order:
 1. Remove SPS_STANDALONE (bug 1317771)
 2. Use Gecko's Mutex
 3. Later, make sure we're not using the reentrancy feature and convert to a non-reentrant Gecko mutex
 4. Much later, once somebody decides they want to revive SPS_STANDALONE, either we have a mutex class in MFBT or we convert to std::mutex which should be easy at that point.

(In reply to Nathan Froyd [:froydnj] from comment #0)
> These are unnecessary, and incur virtual calls during locking (which may be
> able to be elided by sufficiently smart link-time optimization).

Getting rid of this duplicated platform abstraction also makes it easier to add support for other platforms and maybe lets us remove the ENABLE_SPS_PROFLIING ifdefs at some point.

> i.e. SPS_STANDALONE builds are already busted right now, as they're both
> using GeckoMutex.

Huh, this seems to be from the original SPS_STANDALONE patch, so I'm not sure if that patch ever even worked at all in the form that was landed.

Comment 2

[Nicholas Nethercote [inactive]]

(In reply to Markus Stange [:mstange] from comment #1)
> How about attacking this in the following order:
>  1. Remove SPS_STANDALONE (bug 1317771)

I've already taken that bug, so I'll take this one too.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove GeckoMutex, ::Mutex and ::MutexAutoLock from the profiler

Note that the comment on ::Mutex said that it should support recursive locking,
but GeckoMutex was implemented using mozilla::Mutex which does *not* support
recursive locking.

The patch also removes OS::CreateMutex(), because it's only used twice and
doesn't make the code more concise.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove OS::msPerSecond, which is unused

Comment 5

[Nicholas Nethercote [inactive]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=099ce06011a6e788cdbde4a70293a2502c28b2dc

Comment 6

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1140f43c22a1389913b46a7b4a595bcf3fc94b96
Bug 1329684 (part 1) - Remove GeckoMutex, ::Mutex and ::MutexAutoLock from the profiler. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba4bd007828e2b115e8d8acfb9f585512721c036
Bug 1329684 (part 2) - Remove OS::msPerSecond, which is unused. r=mstange.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1140f43c22a1
https://hg.mozilla.org/mozilla-central/rev/ba4bd007828e