1784018 - Remove OSSpinLocks for good

Status: RESOLVED FIXED

(Core :: Memory Allocator, enhancement)

Description

[Gabriele Svelto [:gsvelto]]

Given the feedback from bug 1670885 I decided to find a way to get rid of OSSpinLock even for users of older systems. These are slower machines that are more likely to be under heavy load given the same workload and thus more likely to end up in live-lock situations.

The biggest issues on those machines (running macOS versions between 10.12 and 10.14) is that we can't use OS_UNFAIR_LOCK_ADAPTIVE_SPIN because it's not supported by the older kernels. Given this only affects x86-64 machines however what we can do is mimic what OS_UNFAIR_LOCK_ADAPTIVE_SPIN does but in user-space: that is we spin in user-space for a limited time when the lock is contended but call the pause instruction between each iteration. This will cause another thread running on the same physical core to have all the core's hardware resources available while the other one is spinning. Compared to the kernel-space solution this isn't ideal because there might be no thread on the same core, and the thread holding the lock might be on another core which means we could still be ping-ponging the cache-line holding the lock between the two cores. Still, it's better than making Firefox completely unresponsive while burning power.

Comment 1

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1784018 - Remove deprecated OSSpinLocks r=glandium

On macOS versions prior to 10.15 os_unfair_locks cannot spin in kernel-space
which degrades performance significantly. To obviate for this we spin in
user-space like OSSpinLock does, for the same number of times and invoking
x86-specific pause instructions in-between the locking attempts to avoid
starving a thread that might be running on the same physical core.

Comment 2

[Gabriele Svelto [:gsvelto]]

I'm running tests forcing the new path for macOS 10.14 and older, I probably have to tweak the spin count. Most tests show single-digit performance improvements but the dreaded cross_origin_pageload regresses so I have to address that before this is ready for prime-time.

Comment 3

[Gabriele Svelto [:gsvelto]]

It seems that the spin count I picked (which is the same Apple uses internally) is good enough. Most tests vs OSSpinLock show small improvements, and there's only a handful of smaller regressions:

• talos tests comparison
• browsertime tests comparison

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/02dd305e8ba9
Remove deprecated OSSpinLocks r=glandium

Comment 5

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/02dd305e8ba9

Comment 6

[morgan (Tor Project)]

Is there any chance this patch and related patches could be backported to ESR102?

Full (gecko-dev) patch-list:

https://bugzilla.mozilla.org/show_bug.cgi?id=1670885

• f56fb96281556e7eded0cbb884acf1531eb2049d : Fix post-fork() handlers for PHC/LogAlloc to work on macOS using unfair locks
• 8f9904bb767433e84255b3bc2324d9aadcce3dc2 : Replace deprecated NSSpinLocks with os_unfair_locks in the memory allocator

https://bugzilla.mozilla.org/show_bug.cgi?id=1774458

• 7e8fda1a8bf1a7989a6f44735ff3c3a3d1a8bc46 : Use undocumented, non-public adaptive spinlocks on macOS 10.15+, revert to user-space spinlocks on older versions

https://bugzilla.mozilla.org/show_bug.cgi?id=1774458

• 44cbaf42485d18dad8864addef719c8f3707ad3f : Remove deprecated OSSpinLocks

Thanks!

Comment 7

[Gabriele Svelto [:gsvelto]]

The change has proven to be stable and very much appreciated by our users so maybe yes? Ryan what do you think? IIRC you're the release manager for ESR (please redirect the NI? if I'm wrong).

Comment 8

[Ryan VanderMeulen [:RyanVM]]

This falls pretty far out of scope for an ESR backport, sorry. Tor can always include these patches in their own builds, however.

Comment 9

[morgan (Tor Project)]

Gabriele: are there any other patches we should be aware of if we decide to backport this?

Comment 10

[Gabriele Svelto [:gsvelto]]

(In reply to Richard Pospesel (Tor Browser Dev) from comment #9)

Gabriele: are there any other patches we should be aware of if we decide to backport this?

No, the ones in comment 6 should be enough.