gh-154707: Replace PyThread_type_lock with PyMutex in bufferedio.c

[ByteFlowing1337]

• Issue: Use PyMutex in the buffered I/O module #154707

[ZeroIntensity]

PyThread_type_lock is a PyMutex under the hood, so we don't have to make changes like this.

[bedevere-app]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[ByteFlowing1337]

I think we can still gain some small performance improvements by replacing the legacy APIs, even after #134745?

For example PyThread_allocate_lock calls PyMem_RawMalloc, which may fail and could potentially trigger a slow syscall. Using PyMutex is simpler.

There were also some merged PRs making similar refactors: #150453, #140555, #140711.

[ZeroIntensity]

I'd be very surprised if you noticed any speedup by removing a single 1-byte allocation. What actually prompted you to raise this? Were you benchmarking and found an actual slowdown in the PyMem_RawMalloc used by PyThread_type_lock?

In CPython, we often get contributors who want to make any sort of change solely so they can put "Python contributor" on their CV. This problem has gotten even worse now that LLMs can basically make this effortless. (I'm not accusing you of doing this, just to be clear -- I'm just trying to give you the context that makes me unhappy about this.) As such, we like to avoid "code churn", or in other words, we don't like to make changes that don't have an actual effect on behavior, because:

1. It makes it really easy for drive-by contributors to find PRs to raise without actually making a meaningful contribution to the project.
2. We are a volunteer-run project with a limited number of reviewers, and if we spent our time reviewing cosmetic changes, we'd miss out on many meaningful contributions that people have put a lot of effort into.

Rant aside, my concern is that this will encourage copycat PRs that do the same thing. Again, contributors should make meaningful contributions, even if all they're after is a line on their resume. We don't really care about the intent; we care about the contribution. That means we shouldn't open the door to contributions like this by setting a precedent. I could approve and merge this, and we'd enjoy maybe a 0.001% speedup, but then I risk 10 more PRs that do the same thing in random files across the codebase, and I can't really turn them down for any good reason (beyond just "I said so", which doesn't reflect well on us as a core team).

Those other PRs made it in because they weren't out of the blue; a maintainer noticed it and made the conscious decision to change the code for their own reasons. In particular:

1. gh-150452: use PyMutex in socket module #150453 was because there was an actual bug in that code path, so it made more sense to switch to PyMutex rather than to fix something with a legacy API.
2. gh-116738: Use PyMutex for bz2 module #140555 and gh-116738: Use PyMutex for lzma module #140711 were done because both bz2 and _lzma were being audited for thread-safety bugs in their entirety, and they noticed that the ACQUIRE_LOCK pattern was weird (and looking at the code, it seems it didn't release the GIL when the thread was waiting on the lock?)

When the context is different, the precedent doesn't work as well.

Now, I'm not saying this is a bad change in general. My point is more that this should happen as part of a holistic pass over bufferedio.c or something like that. If you were auditing and fixing thread safety bugs in bufferedio.c and made this change in addition to other real thread safety fixes, then I would be much more comfortable with it. But otherwise, we're just setting a precedent for code churn.