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
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)
Created attachment 8830112 [details] [diff] [review] (part 2) - Remove OS::msPerSecond, which is unused
Attachment #8830112 - 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.
Status: NEW → RESOLVED
Last Resolved: 2 years 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.