Bug report

Bug description:

While testing out the new releases of 3.15, I have noticed that this PR, which I authored, introduced a significant performance regression and I would advise to revert it.

import time
from threading import Thread

THREADS = 4
KEYS = tuple(f"key-{i}" for i in range(16))
UPDATES_PER_THREAD = 1_000_000


def update_dict():
    d = dict.fromkeys(KEYS, 0)

    def worker():
        keys = KEYS
        h = d
        for update in range(UPDATES_PER_THREAD):
            key = keys[update % len(keys)]
            h[key] = update

    workers = [Thread(target=worker) for _ in range(THREADS)]
    for w in workers: w.start()
    for w in workers: w.join()


started = time.perf_counter()
update_dict()
took = time.perf_counter() - started
print(f"{took:.3f}s")

Which outputs 0.507s on main and 0.260s with the revert, on an M4 Max. Note that essentially this is the case where several threads try to write into a shared dictionary, so I wouldn't necessarily call this artificial.

The patch changes the acquisition path of PyMutex to reload the lock state while in the spinning phase, before parking the thread after a number of failed attempts. Prior to my change, the lock state was never reloaded during the spinning phase, which is arguably a bad thing for a spinlock to do. Regardless, reloading is a tradeoff between increasing the contention on the lock state due to the read and promptness of acquisition. Under certain workloads, the tradeoff works in favor of reloading and in others it doesn't, hindering the acquisition rate.

This was always known, but I didn't realize the extent with which this impacts certain scenarios. From what I could observe by running the lockbench.py script under a wide variety of parameter values, the greatest improvement is around 3x more throughput, while the greatest slowdown is around 6x less throughput. I ran this comparison sweep with Codex between main and main with commit
daa159f98b3689ae6a587bfb978b931762f9dbc9 reverted. (See attached pdf for the results.)

I have tried for the past week to work on a patch that would keep the speedups and cope with the slowdowns, but I couldn't find a way to do it.

I believe the mistake made with this patch was looking for a mutex performance improvement without a good enough set of benchmarks, though that would be a big problem to tackle in and of itself. It was thus difficult to see how this would have impacted related code like the one shown. One way to deal with this in the future would be to bruteforce it and try many possible parameter sets for lockbench, as I did in writing this. (Which is easier than coming up with a good set of benchmarks for PyMutex.)

My gut feeling is to follow the principle of least surprise and revert the patch, at least for now. We may evaluate alternative implementations later, possibly once we have broader benchmark coverage.

Reverting would arguably re-open the associated issue, as it pertains this patch, though I'm not sure if it can be fixed given how much reloading hurts performance in some cases. Note that the non-blocking call problem mentioned in the issue was fixed separately and reverting the PR above would not impact it.

CPython versions tested on:

CPython main branch

Operating systems tested on:

macOS

Linked PRs

• gh-157914: Revert "gh-135871: Reload lock internal state while spinning in PyMutex_LockTimed (gh-146064)" #158082
• [3.15] gh-157914: Revert "gh-135871: Reload lock internal state while spinning in PyMutex_LockTimed (gh-146064)" (gh-158082) #158290