gh-146452: Improve locking granularity in pickle's batch_dict_exact#150025
Conversation
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
All commit authors signed the Contributor License Agreement. |
scopreon
changed the title
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
…ing) Replace the coarse critical section wrapping the entire function with fine-grained sections covering only PyDict_Next + Py_INCREF. Also handle PyDict_Next returning 0 in the single-item fast path.
| assert(self->proto > 0); | ||
|
|
||
| dict_size = PyDict_GET_SIZE(obj); | ||
| assert(dict_size); |
There was a problem hiding this comment.
We discussed this in person. This assertion could fail if the dictionary gets mutated by another thread between the size check in the caller and this check. The code should work correctly in this case (it will just emit a SETITEMS that sets 0 items), so removing the assertion is the right change.
JelleZijlstra
added
needs backport to 3.14
|
Thanks @scopreon for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14, 3.15. |
|
Sorry, @scopreon and @JelleZijlstra, I could not cleanly backport this to |
|
GH-150039 is a backport of this pull request to the 3.15 branch. |
bedevere-app
Bot
removed
the
needs backport to 3.15
…exact and fix race condition (GH-150025) (#150039) gh-146452: Improve locking granularity in pickle's batch_dict_exact and fix race condition (GH-150025) Remove assertion that could fail in rare race condition. Replace the coarse critical section wrapping the entire function with fine-grained sections covering only PyDict_Next + Py_INCREF. Also handle PyDict_Next returning 0 in the single-item fast path. (cherry picked from commit 57a0e57) Co-authored-by: Saul Cooperman <58375603+scopreon@users.noreply.github.com>
2 participants
Improve critical section locking in
batch_dict_exact. This originally held a lock for the entirey ofbatch_dict_exact_impl, which is not necessary. We can instead have localised locks for sections which require it. It wrapped possibly expensive calls tosave()etcRemove
batch_dict_exact_implwhich existed to allow the wrapping with critical sections. Called bybatch_dict_exact.Additionally handle the case where
PyDict_Nextreturns 0 in the size 1 special case (modified during pickling).Note: this spun out of looking into #149816 89, it was fixed in #146452 but I'm not sure exactly what issue this belongs to.
Test was added in https://github.com/python/cpython/pull/146470/changes