remove ::Mutex/::MutexAutoLock wrappers

RESOLVED FIXED in Firefox 54

Status

()

Core
General
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: froydnj, Assigned: njn)

Tracking

(Blocks: 1 bug)

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 months ago
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...
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.
(Assignee)

Comment 2

3 months ago
(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.
Assignee: nobody → n.nethercote
(Assignee)

Comment 3

3 months ago
Created attachment 8830110 [details] [diff] [review]
(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.
Attachment #8830110 - Flags: review?(mstange)
(Assignee)

Comment 4

3 months ago
Created attachment 8830112 [details] [diff] [review]
(part 2) - Remove OS::msPerSecond, which is unused
Attachment #8830112 - Flags: review?(mstange)
(Assignee)

Comment 5

3 months ago
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=099ce06011a6e788cdbde4a70293a2502c28b2dc
Attachment #8830110 - Flags: review?(mstange) → review+
Attachment #8830112 - Flags: review?(mstange) → review+
(Assignee)

Comment 6

3 months ago
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

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1140f43c22a1
https://hg.mozilla.org/mozilla-central/rev/ba4bd007828e
Status: NEW → RESOLVED
Last Resolved: 3 months 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.