Closed
Bug 1429665
Opened 6 years ago
Closed 6 years ago
ipc::mscom::SpinEvent should only spin for a short time
Categories
(Core :: IPC: MSCOM, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: Jamie, Assigned: Jamie)
Details
Attachments
(1 file)
Bug 1429665: ipc::mscom::SpinEvent: Only spin for a short time, falling back to an event thereafter.
59 bytes,
text/x-review-board-request
|
bugzilla
:
review+
|
Details |
SpinEvent was introduced to facilitate fast response (on multi-processor systems) when waiting on another thread for something that might return faster than the system's timer resolution. This is important for accessibility, since we don't want lots of calls potentially waiting a full timer tick and thus slowing down responsiveness for the user. However, in a deadlock situation such as bug 1428759, this causes the spin to dominate the processor, thus rendering the system extremely difficult to use. (Obviously, we want to fix deadlocks, but even debugging them becomes extremely difficult because of this.) Furthermore, for calls that don't return immediately, we don't want to waste lots of CPU cycles on spinning. Once you get past a certain point, a few extra ms isn't going to make that much difference to perceived responsiveness. We should modify SpinEvent so that on multi-processor systems, it spins only for a short time; e.g. 30 ms. After that, it should fall back to waiting on an event, just as we do on uni-processor. While we could query the system to get the timer resolution, this increases complexity and I don't think there's any advantage; we just want to choose a reasonable value for a "fast" call.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jteh
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•6 years ago
|
||
I figured I may as well deal with this while I'm able to reproduce bug 1428759, since that's what initially triggered my concern about SpinEvent. I tested this by reproducing bug 1428759 before and after the patch. Before, my system's CPU load was 20% or higher (sometimes much higher) during the deadlock. (The load probably depends on how many calls were waiting.) After the patch, the CPU load stayed lower than 10%. To ensure we weren't always falling back to the event, I tested with some debug output where the event was used. If we were always falling back, I expected to be spammed. Instead, I only saw a few instances where the event was used. I also did some quick usage testing on a single core vm to make sure everything worked as expected.
Comment 3•6 years ago
|
||
mozreview-review |
Comment on attachment 8942573 [details] Bug 1429665: ipc::mscom::SpinEvent: Only spin for a short time, falling back to an event thereafter. https://reviewboard.mozilla.org/r/212828/#review239884 ::: ipc/mscom/SpinEvent.cpp:46 (Diff revision 1) > +{ > + static const bool gotStatics = InitStatics(); > + MOZ_ASSERT(gotStatics); > > - if (!sIsMulticore) { > - mDoneEvent.own(::CreateEventW(nullptr, FALSE, FALSE, nullptr)); > + mDoneEvent.own(::CreateEventW(nullptr, FALSE, FALSE, nullptr)); Ideally we'd only create the event as necessary, but that introduces a lot of additional concurrency headaches that I'd rather we didn't deal with at the moment. ::: ipc/mscom/SpinEvent.cpp:70 (Diff revision 1) > + TimeStamp start(TimeStamp::Now()); > - while (!mDone) { > + while (!mDone) { > - // The PAUSE instruction is a hint to the CPU that we're doing a spin > + // The PAUSE instruction is a hint to the CPU that we're doing a spin > - // loop. It is a no-op on older processors that don't support it, so > + // loop. It is a no-op on older processors that don't support it, so > - // it is safe to use here without any CPUID checks. > + // it is safe to use here without any CPUID checks. > - CPU_PAUSE(); > + CPU_PAUSE(); Move this so that CPU_PAUSE is the last instruction inside the loop.
Attachment #8942573 -
Flags: review?(aklotz) → review+
Comment hidden (mozreview-request) |
Pushed by jteh@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cedf0be06827 ipc::mscom::SpinEvent: Only spin for a short time, falling back to an event thereafter. r=aklotz
Comment 6•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cedf0be06827
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
status-firefox59:
affected → ---
Comment 7•6 years ago
|
||
Performance wins! == Change summary for alert #12553 (as of Fri, 06 Apr 2018 01:39:26 GMT) == Improvements: 7% Strings PerfUTF16toUTF8THThousand windows7-32 opt 261,017.23 -> 243,656.92 6% Strings PerfUTF16toUTF8JAHundred windows7-32 opt 34,785.42 -> 32,817.00 4% Strings PerfUTF16toUTF8KOHundred windows7-32 opt 31,574.17 -> 30,362.75 4% Strings PerfUTF16toUTF8THFifteen windows7-32 opt 6,590.67 -> 6,345.42 4% Strings PerfUTF16toUTF8THHundred windows7-32 opt 33,074.42 -> 31,866.92 3% Strings PerfUTF16toUTF8JAFifteen windows7-32 opt 6,448.15 -> 6,224.83 3% Strings PerfUTF8toUTF16VIFifteen windows7-32 opt 9,659.77 -> 9,412.50 2% Strings PerfUTF16toUTF8KOFifteen windows7-32 opt 5,752.23 -> 5,627.83 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=12553
You need to log in
before you can comment on or make changes to this bug.
Description
•