fix: better VM exit detection for periodic threads [backport 4.5]#16901
Conversation
Codeowners resolved as |
KowalskiThomas
changed the title
We improve our detection of the VM shutdown when tearing down our periodic threads. We cannot rely on retrieving the finalisation state because we cannot lock around it, so we have to set a module-state barrier that we can more reliably use to detect when it's no longer safe to interact with the VM.
The new behaviour is described by the following timeline, where at the top we have our code, and at the bottom the VM internal operations.
```
Periodic threads
are stopped Barrier
┏─────────┓ ┃
┈┈──────┰──────┰─┸─────────┸───────────┸────┰──────┰─💀💀💀💀─┈┈
┃ ┗────────────────────────────┛ ┃
shutdown at-exit hooks run finalizing
triggered flag set
```
From the timeline we see that we make sure that all our threads have completed during the invocation of the `atexit` hooks. These complete before the VM sets its internal `finalizing` flag, after which some C API calls become unsafe. We also set a Boolean barrier during the `atexit` hook as part of the `threads` module state. Whilst not strictly necessary with the new behaviour, it gives us extra assurances that we won't make any unsafe calls after the `finalizing` flag has been set. In cases where the `atexit` hooks are not executed, we fallback to the non-thread-safe `(_)Py_IsFinalizing`.
The original implementation relied on a per-thread barrier and `(_)Py_IsFinalizing` for VM shutdown detection. Furthermore, we also used custom signals to implement the logic for joining threads. This made us vulnerable to a race whereby a thread could actually shutdown moments after the internal `finalizing` flag was set, and go on to invoke the destructors of `GILGuard` and `PyRef` via RAII. We make sure that we wait until the threads has completed its full execution by detaching only after a fork. This way we are guaranteed to have all periodic threads stopped _before_ `finalizing` is set.
Co-authored-by: thomas.kowalski <[email protected]>
(cherry picked from commit ae53696)
0b3644a to
3ca672e
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b3644a4c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
emmettbutler
left a comment
emmettbutler
left a comment
There was a problem hiding this comment.
Why is this critical enough to backport?
It's causing a significant number of crashes, so I'd say yes? |
Performance SLOsComparing candidate kowalski/fix-better-vm-exit-detection-for-periodic-threads-16741 (3ca672e) with baseline 4.5 (6af3056) 📈 Performance Regressions (2 suites)📈 iastaspects - 118/118✅ add_aspectTime: ✅ 104.257µs (SLO: <130.000µs 📉 -19.8%) vs baseline: -0.2% Memory: ✅ 42.939MB (SLO: <46.000MB -6.7%) vs baseline: +5.0% ✅ add_inplace_aspectTime: ✅ 104.092µs (SLO: <130.000µs 📉 -19.9%) vs baseline: -0.4% Memory: ✅ 42.959MB (SLO: <46.000MB -6.6%) vs baseline: +4.9% ✅ add_inplace_noaspectTime: ✅ 28.618µs (SLO: <40.000µs 📉 -28.5%) vs baseline: +1.8% Memory: ✅ 42.939MB (SLO: <46.000MB -6.7%) vs baseline: +5.0% ✅ add_noaspectTime: ✅ 49.195µs (SLO: <70.000µs 📉 -29.7%) vs baseline: +0.9% Memory: ✅ 43.018MB (SLO: <46.000MB -6.5%) vs baseline: +5.0% ✅ bytearray_aspectTime: ✅ 246.403µs (SLO: <400.000µs 📉 -38.4%) vs baseline: -0.6% Memory: ✅ 42.939MB (SLO: <46.000MB -6.7%) vs baseline: +5.0% ✅ bytearray_extend_aspectTime: ✅ 642.212µs (SLO: <800.000µs 📉 -19.7%) vs baseline: +0.4% Memory: ✅ 42.900MB (SLO: <46.000MB -6.7%) vs baseline: +4.8% ✅ bytearray_extend_noaspectTime: ✅ 263.434µs (SLO: <400.000µs 📉 -34.1%) vs baseline: -0.9% Memory: ✅ 42.998MB (SLO: <46.000MB -6.5%) vs baseline: +5.1% ✅ bytearray_noaspectTime: ✅ 139.683µs (SLO: <300.000µs 📉 -53.4%) vs baseline: +2.2% Memory: ✅ 43.037MB (SLO: <46.000MB -6.4%) vs baseline: +5.1% ✅ bytes_aspectTime: ✅ 216.617µs (SLO: <300.000µs 📉 -27.8%) vs baseline: +0.5% Memory: ✅ 42.979MB (SLO: <46.000MB -6.6%) vs baseline: +5.0% ✅ bytes_noaspectTime: ✅ 133.133µs (SLO: <200.000µs 📉 -33.4%) vs baseline: +0.4% Memory: ✅ 42.959MB (SLO: <46.000MB -6.6%) vs baseline: +5.0% ✅ bytesio_aspectTime: ✅ 3.769ms (SLO: <5.000ms 📉 -24.6%) vs baseline: +0.6% Memory: ✅ 42.979MB (SLO: <46.000MB -6.6%) vs baseline: +5.0% ✅ bytesio_noaspectTime: ✅ 314.611µs (SLO: <420.000µs 📉 -25.1%) vs baseline: ~same Memory: ✅ 42.939MB (SLO: <46.000MB -6.7%) vs baseline: +4.8% ✅ capitalize_aspectTime: ✅ 88.109µs (SLO: <300.000µs 📉 -70.6%) vs baseline: -0.4% Memory: ✅ 42.998MB (SLO: <46.000MB -6.5%) vs baseline: +5.2% ✅ capitalize_noaspectTime: ✅ 249.981µs (SLO: <300.000µs 📉 -16.7%) vs baseline: ~same Memory: ✅ 42.920MB (SLO: <46.000MB -6.7%) vs baseline: +4.8% ✅ casefold_aspectTime: ✅ 88.522µs (SLO: <500.000µs 📉 -82.3%) vs baseline: +0.4% Memory: ✅ 43.037MB (SLO: <46.000MB -6.4%) vs baseline: +5.2% ✅ casefold_noaspectTime: ✅ 300.640µs (SLO: <500.000µs 📉 -39.9%) vs baseline: -0.6% Memory: ✅ 42.920MB (SLO: <46.000MB -6.7%) vs baseline: +4.8% ✅ decode_aspectTime: ✅ 86.519µs (SLO: <100.000µs 📉 -13.5%) vs baseline: +0.1% Memory: ✅ 42.939MB (SLO: <46.000MB -6.7%) vs baseline: +4.9% ✅ decode_noaspectTime: ✅ 152.607µs (SLO: <210.000µs 📉 -27.3%) vs baseline: +0.4% Memory: ✅ 42.959MB (SLO: <46.000MB -6.6%) vs baseline: +5.0% ✅ encode_aspectTime: ✅ 84.216µs (SLO: <200.000µs 📉 -57.9%) vs baseline: -0.3% Memory: ✅ 42.979MB (SLO: <46.000MB -6.6%) vs baseline: +5.0% ✅ encode_noaspectTime: ✅ 143.463µs (SLO: <200.000µs 📉 -28.3%) vs baseline: +4.0% Memory: ✅ 42.939MB (SLO: <46.000MB -6.7%) vs baseline: +5.0% ✅ format_aspectTime: ✅ 14.618ms (SLO: <19.200ms 📉 -23.9%) vs baseline: ~same Memory: ✅ 42.959MB (SLO: <46.000MB -6.6%) vs baseline: +4.8% ✅ format_map_aspectTime: ✅ 16.380ms (SLO: <21.500ms 📉 -23.8%) vs baseline: -0.4% Memory: ✅ 42.998MB (SLO: <46.000MB -6.5%) vs baseline: +4.7% ✅ format_map_noaspectTime: ✅ 370.386µs (SLO: <500.000µs 📉 -25.9%) vs baseline: +1.0% Memory: ✅ 42.959MB (SLO: <46.000MB -6.6%) vs baseline: +4.9% ✅ format_noaspectTime: ✅ 300.443µs (SLO: <500.000µs 📉 -39.9%) vs baseline: -1.9% Memory: ✅ 42.900MB (SLO: <46.000MB -6.7%) vs baseline: +4.7% ✅ index_aspectTime: ✅ 127.770µs (SLO: <300.000µs 📉 -57.4%) vs baseline: +6.5% Memory: ✅ 42.979MB (SLO: <46.000MB -6.6%) vs baseline: +5.0% ✅ index_noaspectTime: ✅ 39.969µs (SLO: <300.000µs 📉 -86.7%) vs baseline: -0.3% Memory: ✅ 42.900MB (SLO: <46.000MB -6.7%) vs baseline: +4.8% ✅ join_aspectTime: ✅ 206.716µs (SLO: <300.000µs 📉 -31.1%) vs baseline: -1.9% Memory: ✅ 42.959MB (SLO: <46.000MB -6.6%) vs baseline: +4.9% ✅ join_noaspectTime: ✅ 144.765µs (SLO: <300.000µs 📉 -51.7%) vs baseline: +2.1% Memory: ✅ 42.979MB (SLO: <46.000MB -6.6%) vs baseline: +5.0% ✅ ljust_aspectTime: ✅ 566.622µs (SLO: <700.000µs 📉 -19.1%) vs baseline: 📈 +14.5% Memory: ✅ 42.900MB (SLO: <46.000MB -6.7%) vs baseline: +4.8% ✅ ljust_noaspectTime: ✅ 255.946µs (SLO: <300.000µs 📉 -14.7%) vs baseline: +1.2% Memory: ✅ 42.959MB (SLO: <46.000MB -6.6%) vs baseline: +5.0% ✅ lower_aspectTime: ✅ 290.503µs (SLO: <500.000µs 📉 -41.9%) vs baseline: -0.4% Memory: ✅ 42.979MB (SLO: <46.000MB -6.6%) vs baseline: +4.9% ✅ lower_noaspectTime: ✅ 230.242µs (SLO: <300.000µs 📉 -23.3%) vs baseline: -2.3% Memory: ✅ 42.920MB (SLO: <46.000MB -6.7%) vs baseline: +4.9% ✅ lstrip_aspectTime: ✅ 0.266ms (SLO: <3.000ms 📉 -91.1%) vs baseline: -1.1% Memory: ✅ 42.920MB (SLO: <46.000MB -6.7%) vs baseline: +4.9% ✅ lstrip_noaspectTime: ✅ 0.173ms (SLO: <3.000ms 📉 -94.2%) vs baseline: -1.4% Memory: ✅ 42.939MB (SLO: <46.000MB -6.7%) vs baseline: +4.8% ✅ modulo_aspectTime: ✅ 14.281ms (SLO: <18.750ms 📉 -23.8%) vs baseline: ~same Memory: ✅ 43.018MB (SLO: <46.000MB -6.5%) vs baseline: +4.6% ✅ modulo_aspect_for_bytearray_bytearrayTime: ✅ 14.779ms (SLO: <19.350ms 📉 -23.6%) vs baseline: -0.3% Memory: ✅ 43.037MB (SLO: <46.000MB -6.4%) vs baseline: +5.0% ✅ modulo_aspect_for_bytesTime: ✅ 14.359ms (SLO: <18.900ms 📉 -24.0%) vs baseline: ~same Memory: ✅ 43.018MB (SLO: <46.000MB -6.5%) vs baseline: +4.7% ✅ modulo_aspect_for_bytes_bytearrayTime: ✅ 14.581ms (SLO: <19.150ms 📉 -23.9%) vs baseline: -0.8% Memory: ✅ 42.998MB (SLO: <46.000MB -6.5%) vs baseline: +4.8% ✅ modulo_noaspectTime: ✅ 0.357ms (SLO: <3.000ms 📉 -88.1%) vs baseline: -1.9% Memory: ✅ 42.959MB (SLO: <46.000MB -6.6%) vs baseline: +5.0% ✅ replace_aspectTime: ✅ 18.449ms (SLO: <24.000ms 📉 -23.1%) vs baseline: +0.4% Memory: ✅ 42.998MB (SLO: <46.000MB -6.5%) vs baseline: +4.8% ✅ replace_noaspectTime: ✅ 278.400µs (SLO: <300.000µs -7.2%) vs baseline: -1.8% Memory: ✅ 42.920MB (SLO: <46.000MB -6.7%) vs baseline: +4.8% ✅ repr_aspectTime: ✅ 311.481µs (SLO: <420.000µs 📉 -25.8%) vs baseline: +0.5% Memory: ✅ 43.057MB (SLO: <46.000MB -6.4%) vs baseline: +5.2% ✅ repr_noaspectTime: ✅ 46.885µs (SLO: <90.000µs 📉 -47.9%) vs baseline: -0.7% Memory: ✅ 42.920MB (SLO: <46.000MB -6.7%) vs baseline: +5.0% ✅ rstrip_aspectTime: ✅ 380.197µs (SLO: <500.000µs 📉 -24.0%) vs baseline: -0.6% Memory: ✅ 42.939MB (SLO: <46.000MB -6.7%) vs baseline: +4.9% ✅ rstrip_noaspectTime: ✅ 181.111µs (SLO: <300.000µs 📉 -39.6%) vs baseline: +0.9% Memory: ✅ 42.959MB (SLO: <46.000MB -6.6%) vs baseline: +4.8% ✅ slice_aspectTime: ✅ 180.354µs (SLO: <300.000µs 📉 -39.9%) vs baseline: -1.2% Memory: ✅ 42.959MB (SLO: <46.000MB -6.6%) vs baseline: +5.0% ✅ slice_noaspectTime: ✅ 53.928µs (SLO: <90.000µs 📉 -40.1%) vs baseline: +0.6% Memory: ✅ 42.959MB (SLO: <46.000MB -6.6%) vs baseline: +4.9% ✅ stringio_aspectTime: ✅ 4.336ms (SLO: <5.000ms 📉 -13.3%) vs baseline: 📈 +13.9% Memory: ✅ 42.920MB (SLO: <46.000MB -6.7%) vs baseline: +4.9% ✅ stringio_noaspectTime: ✅ 345.247µs (SLO: <500.000µs 📉 -31.0%) vs baseline: +0.7% Memory: ✅ 42.939MB (SLO: <46.000MB -6.7%) vs baseline: +4.9% ✅ strip_aspectTime: ✅ 268.410µs (SLO: <350.000µs 📉 -23.3%) vs baseline: ~same Memory: ✅ 42.920MB (SLO: <46.000MB -6.7%) vs baseline: +4.9% ✅ strip_noaspectTime: ✅ 173.830µs (SLO: <240.000µs 📉 -27.6%) vs baseline: -0.1% Memory: ✅ 42.979MB (SLO: <46.000MB -6.6%) vs baseline: +5.0% ✅ swapcase_aspectTime: ✅ 325.417µs (SLO: <500.000µs 📉 -34.9%) vs baseline: -1.5% Memory: ✅ 42.998MB (SLO: <46.000MB -6.5%) vs baseline: +5.2% ✅ swapcase_noaspectTime: ✅ 267.044µs (SLO: <400.000µs 📉 -33.2%) vs baseline: -1.8% Memory: ✅ 42.998MB (SLO: <46.000MB -6.5%) ✅ title_aspectTime: ✅ 318.609µs (SLO: <500.000µs 📉 -36.3%) vs baseline: +0.3% Memory: ✅ 42.979MB (SLO: <46.000MB -6.6%) vs baseline: +5.0% ✅ title_noaspectTime: ✅ 253.138µs (SLO: <400.000µs 📉 -36.7%) vs baseline: -2.0% Memory: ✅ 43.037MB (SLO: <46.000MB -6.4%) vs baseline: +5.1% ✅ translate_aspectTime: ✅ 488.970µs (SLO: <700.000µs 📉 -30.1%) vs baseline: +0.2% Memory: ✅ 42.920MB (SLO: <46.000MB -6.7%) vs baseline: +4.9% ✅ translate_noaspectTime: ✅ 420.611µs (SLO: <500.000µs 📉 -15.9%) vs baseline: -0.8% Memory: ✅ 42.920MB (SLO: <46.000MB -6.7%) vs baseline: +5.0% ✅ upper_aspectTime: ✅ 291.516µs (SLO: <500.000µs 📉 -41.7%) vs baseline: -0.3% Memory: ✅ 42.900MB (SLO: <46.000MB -6.7%) vs baseline: +4.8% ✅ upper_noaspectTime: ✅ 229.923µs (SLO: <400.000µs 📉 -42.5%) vs baseline: -2.2% Memory: ✅ 42.939MB (SLO: <46.000MB -6.7%) vs baseline: +4.8% 📈 iastaspectsospath - 24/24✅ ospathbasename_aspectTime: ✅ 509.517µs (SLO: <700.000µs 📉 -27.2%) vs baseline: 📈 +23.0% Memory: ✅ 42.959MB (SLO: <46.000MB -6.6%) vs baseline: +4.9% ✅ ospathbasename_noaspectTime: ✅ 424.587µs (SLO: <700.000µs 📉 -39.3%) vs baseline: -0.9% Memory: ✅ 43.037MB (SLO: <46.000MB -6.4%) vs baseline: +5.1% ✅ ospathjoin_aspectTime: ✅ 616.911µs (SLO: <700.000µs 📉 -11.9%) vs baseline: -0.6% Memory: ✅ 42.959MB (SLO: <46.000MB -6.6%) vs baseline: +5.0% ✅ ospathjoin_noaspectTime: ✅ 626.149µs (SLO: <700.000µs 📉 -10.6%) vs baseline: -0.9% Memory: ✅ 42.959MB (SLO: <46.000MB -6.6%) vs baseline: +5.0% ✅ ospathnormcase_aspectTime: ✅ 347.139µs (SLO: <700.000µs 📉 -50.4%) vs baseline: +0.1% Memory: ✅ 42.880MB (SLO: <46.000MB -6.8%) vs baseline: +4.8% ✅ ospathnormcase_noaspectTime: ✅ 359.883µs (SLO: <700.000µs 📉 -48.6%) vs baseline: +1.1% Memory: ✅ 42.979MB (SLO: <46.000MB -6.6%) vs baseline: +4.9% ✅ ospathsplit_aspectTime: ✅ 485.399µs (SLO: <700.000µs 📉 -30.7%) vs baseline: +1.7% Memory: ✅ 42.939MB (SLO: <46.000MB -6.7%) vs baseline: +5.0% ✅ ospathsplit_noaspectTime: ✅ 492.394µs (SLO: <700.000µs 📉 -29.7%) vs baseline: -0.3% Memory: ✅ 42.920MB (SLO: <46.000MB -6.7%) vs baseline: +4.8% ✅ ospathsplitdrive_aspectTime: ✅ 372.573µs (SLO: <700.000µs 📉 -46.8%) vs baseline: +0.8% Memory: ✅ 42.880MB (SLO: <46.000MB -6.8%) vs baseline: +4.6% ✅ ospathsplitdrive_noaspectTime: ✅ 73.020µs (SLO: <700.000µs 📉 -89.6%) vs baseline: -0.3% Memory: ✅ 42.959MB (SLO: <46.000MB -6.6%) vs baseline: +5.0% ✅ ospathsplitext_aspectTime: ✅ 450.915µs (SLO: <700.000µs 📉 -35.6%) vs baseline: +0.2% Memory: ✅ 42.959MB (SLO: <46.000MB -6.6%) vs baseline: +5.0% ✅ ospathsplitext_noaspectTime: ✅ 461.508µs (SLO: <700.000µs 📉 -34.1%) vs baseline: +0.2% Memory: ✅ 42.920MB (SLO: <46.000MB -6.7%) vs baseline: +4.9%
|
6 participants
Description
This is a backport for #16741 to 4.5.
Depends on #16900.