Closed Bug 1329684 Opened 7 years ago Closed 7 years ago

remove ::Mutex/::MutexAutoLock wrappers

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: froydnj, Assigned: n.nethercote)

References

Details

Attachments

(2 files)

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.
(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
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)
Attachment #8830110 - Flags: review?(mstange) → review+
Attachment #8830112 - Flags: review?(mstange) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/1140f43c22a1
https://hg.mozilla.org/mozilla-central/rev/ba4bd007828e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: