gh-128137: Update PyASCIIObject to handle interned field with the atomic operation #128196

corona10 · 2024-12-23T15:31:14Z

Issue: Race in PyUnicode_InternFromString under free-threading #128137

corona10 · 2024-12-23T15:32:56Z

@colesbury I am not sure what you intended.
To maintain the original size, I squeeze the state field from 32 bits to 16 bits and reserve 16 bits for the interned field.

Include/cpython/unicodeobject.h

corona10 · 2024-12-23T16:03:34Z

f30b355 breaks WASI test: https://github.com/python/cpython/actions/runs/12469477403/job/34802702354?pr=128196 (object size is increased).

colesbury · 2024-12-23T16:09:59Z

Include/cpython/unicodeobject.h

+        3: Interned, Immortal, and Static
+    This categorization allows the runtime to determine the right
+    cleanup mechanism at runtime shutdown. */
+    uint8_t interned;


I think this remain in the state struct. It's okay for a struct to contain both non-bitfield and bitfield members:

It avoids a potential unnecessary breakage from moving the field

Keeping it in state will make it easier to keep state 32-bits due to alignment.

Include/cpython/unicodeobject.h

colesbury · 2024-12-23T16:26:18Z

I think this should have a NEWS entry given that we're changing the size of a semi-public field.

corona10 · 2024-12-23T16:51:14Z

Since we touch the semi-public field, I am not sure about backporting this PR.
Let's just leave 3.13t as buggy status because it is just the experimental build.

bedevere-bot · 2025-01-02T03:02:18Z

🤖 New build scheduled with the buildbot fleet by @corona10 for commit 91907aa 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

corona10 · 2025-01-02T04:22:17Z

I think that it would be fine

uint16_t: interned field = 16bits (Increase it to 16bits is little bit painful, but it would be nothing for modern CPUs)
unsigned short: kind + compact + ascii + statically_allocated + padding = 16bits (for 32bits and 64bits system both)
In total: 32 bits

corona10 · 2025-01-02T04:26:26Z

In Linux

sizeof PyASCIIObject is 40bytes
sizeof PyASCIIObject.state is 4 bytes.

So no regression will be.

colesbury

This LGTM. @methane, would you please take a look at this as well?

…he atomic operation (pythongh-128196)

Due to python/cpython#128196, data types within `PyASCIIObject.state` have changed, resulting in test failures when building against 3.14.

* Add news item * Move news file * Fix version limit check in noxfile.py * Bump Python version for testing debug builds * 3.14 is available from GH's setup-python action * Bump maximum supported CPython version in pyo3-ffi * Rework PyASCIIObject and PyUnicodeObject to be compatible with 3.14 Due to python/cpython#128196, data types within `PyASCIIObject.state` have changed, resulting in test failures when building against 3.14. * Run `cargo fmt --all` * Actually add Py_3_14 as a legitimate macro When `rustc` is invoked, the macro is included with the `--check-cfg` flag, but not with the `--cfg` flag. This caused errors about duplicate definitions to spew out when building with stable Rust toolchains. * Revert "Actually add Py_3_14 as a legitimate macro" This reverts commit 5da57af. * Fix version macro placement for 3.14-specific getters and setters * Import 'c_ushort' only if compiling against CPython 3.14 or later * Add wrapper functions for the statically_allocated field * Remove unused libc::c_ushort * Add (hopefully) final version-specific macros * Port 3.14-specific 64-bit code of Py_INCREF * Don't expose PyDictObject.ma_version_tag when building against 3.14 or later * fix ffi-check on the GIL-enabled ABI * fix older pythons * fix ffi-check on older pythons * WIP: update for 3.14t * fix ffi-check on the free-threaded build * fix clippy * fix clippy on older python versions * fix cargo check on the MSRV * fix ffi-check on 3.13t * fix CI which is using 3.13.1 * fix copy/paste error in noxfile * update ffi bindings for the latest changes in 3.14 * update layout of refcnt field on gil-enabled build * delete unused HangThread struct * fix ffi-check on GIL-enabled build * Revert "delete unused HangThread struct" This reverts commit 3dd439d. * config-out HangThread * fix 3.13 ffi-check * fix debug python build error * fix graalpy build * Ignore DeprecationWarnings from the pytest_asyncio module in tests * Add abi3-py314 * fix free-threading issue in `test_coroutine` (#5069) * Introspection: add function signatures (#5025) * Introspection: add function signatures No annotations or explicit default values yet Fixes an issue related to object identifiers path * Better default value * Refine arguments struct * Introduce VariableLengthArgument * Adds pyfunctions tests * Adds some serialization tests * respond to david's code review * add comment and fix test failure * fix check-feature-powerset * fix clippy * fix wasip1 clippy * fix 32 bit python 3.14 bug * mark test-py step continue-on-error for dev python builds * use github issue URL * run ffi-check before running tests * fix ffi-check for 3.14.0a7 --------- Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com> Co-authored-by: David Hewitt <mail@davidhewitt.dev> Co-authored-by: Thomas Tanon <thomas.pellissier-tanon@helsing.ai>

* Add news item * Move news file * Fix version limit check in noxfile.py * Bump Python version for testing debug builds * 3.14 is available from GH's setup-python action * Bump maximum supported CPython version in pyo3-ffi * Rework PyASCIIObject and PyUnicodeObject to be compatible with 3.14 Due to python/cpython#128196, data types within `PyASCIIObject.state` have changed, resulting in test failures when building against 3.14. * Run `cargo fmt --all` * Actually add Py_3_14 as a legitimate macro When `rustc` is invoked, the macro is included with the `--check-cfg` flag, but not with the `--cfg` flag. This caused errors about duplicate definitions to spew out when building with stable Rust toolchains. * Revert "Actually add Py_3_14 as a legitimate macro" This reverts commit 5da57af. * Fix version macro placement for 3.14-specific getters and setters * Import 'c_ushort' only if compiling against CPython 3.14 or later * Add wrapper functions for the statically_allocated field * Remove unused libc::c_ushort * Add (hopefully) final version-specific macros * Port 3.14-specific 64-bit code of Py_INCREF * Don't expose PyDictObject.ma_version_tag when building against 3.14 or later * fix ffi-check on the GIL-enabled ABI * fix older pythons * fix ffi-check on older pythons * WIP: update for 3.14t * fix ffi-check on the free-threaded build * fix clippy * fix clippy on older python versions * fix cargo check on the MSRV * fix ffi-check on 3.13t * fix CI which is using 3.13.1 * fix copy/paste error in noxfile * update ffi bindings for the latest changes in 3.14 * update layout of refcnt field on gil-enabled build * delete unused HangThread struct * fix ffi-check on GIL-enabled build * Revert "delete unused HangThread struct" This reverts commit 3dd439d. * config-out HangThread * fix 3.13 ffi-check * fix debug python build error * fix graalpy build * Ignore DeprecationWarnings from the pytest_asyncio module in tests * Add abi3-py314 * fix free-threading issue in `test_coroutine` (PyO3#5069) * Introspection: add function signatures (PyO3#5025) * Introspection: add function signatures No annotations or explicit default values yet Fixes an issue related to object identifiers path * Better default value * Refine arguments struct * Introduce VariableLengthArgument * Adds pyfunctions tests * Adds some serialization tests * respond to david's code review * add comment and fix test failure * fix check-feature-powerset * fix clippy * fix wasip1 clippy * fix 32 bit python 3.14 bug * mark test-py step continue-on-error for dev python builds * use github issue URL * run ffi-check before running tests * fix ffi-check for 3.14.0a7 --------- Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com> Co-authored-by: David Hewitt <mail@davidhewitt.dev> Co-authored-by: Thomas Tanon <thomas.pellissier-tanon@helsing.ai>

pythongh-128137: Split out interned field from state

45536d0

corona10 added the skip news label Dec 23, 2024

corona10 requested review from vstinner, mpage and colesbury December 23, 2024 15:31

bedevere-app bot mentioned this pull request Dec 23, 2024

Race in PyUnicode_InternFromString under free-threading #128137

Closed

bedevere-app bot added the awaiting core review label Dec 23, 2024

corona10 added the topic-free-threading label Dec 23, 2024

kumaraditya303 reviewed Dec 23, 2024

View reviewed changes

Include/cpython/unicodeobject.h Outdated Show resolved Hide resolved

colesbury reviewed Dec 23, 2024

View reviewed changes

Include/cpython/unicodeobject.h Outdated Show resolved Hide resolved

Include/cpython/unicodeobject.h Outdated Show resolved Hide resolved

Address code review

f30b355

corona10 requested a review from colesbury December 23, 2024 15:54

corona10 commented Dec 23, 2024

View reviewed changes

Include/cpython/unicodeobject.h Outdated Show resolved Hide resolved

Maintain unused size as 24 bits

18a8e6b

colesbury reviewed Dec 23, 2024

View reviewed changes

Reserve unused bit as 18 bits

5c659ef

corona10 changed the title ~~gh-128137: Split out interned field from state~~ [WIP] gh-128137: Split out interned field from state Dec 23, 2024

corona10 added 3 commits December 24, 2024 01:34

Address code review

c803475

nit

2dfd450

nit

69ad224

corona10 changed the title ~~[WIP] gh-128137: Split out interned field from state~~ gh-128137: Split out interned field from state Dec 23, 2024

corona10 removed the skip news label Dec 23, 2024

Add NEWS.d

b1b396b

corona10 changed the title ~~gh-128137: Split out interned field from state~~ gh-128137: Update PyASCIIObject layout to handle interned field with the atomic operation. Dec 23, 2024

corona10 changed the title ~~gh-128137: Update PyASCIIObject layout to handle interned field with the atomic operation.~~ gh-128137: Update PyASCIIObject to handle interned field with the atomic operation Dec 23, 2024

corona10 requested a review from colesbury December 23, 2024 16:46

corona10 added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 2, 2025

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 2, 2025

colesbury approved these changes Jan 2, 2025

View reviewed changes

methane approved these changes Jan 5, 2025

View reviewed changes

corona10 merged commit ae23a01 into python:main Jan 5, 2025
118 checks passed

bedevere-app bot removed the awaiting merge label Jan 5, 2025

corona10 deleted the gh-128137 branch January 5, 2025 09:17

srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 6, 2025

pythongh-128137: Update PyASCIIObject to handle interned field with t…

2f8b072

…he atomic operation (pythongh-128196)

srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 8, 2025

pythongh-128137: Update PyASCIIObject to handle interned field with t…

7a47ab7

…he atomic operation (pythongh-128196)

ZeroIntensity mentioned this pull request Jan 18, 2025

Incompatible change in internal string representation (encountered with Rust bindings) (?) #128972

Open

colesbury mentioned this pull request Feb 13, 2025

Data race in intern_common when interning str objects in the free threading build #129701

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-128137: Update PyASCIIObject to handle interned field with the atomic operation #128196

gh-128137: Update PyASCIIObject to handle interned field with the atomic operation #128196

corona10 commented Dec 23, 2024 •

edited by bedevere-app bot

Loading

corona10 commented Dec 23, 2024

corona10 commented Dec 23, 2024

colesbury Dec 23, 2024

colesbury commented Dec 23, 2024

corona10 commented Dec 23, 2024 •

edited

Loading

bedevere-bot commented Jan 2, 2025

corona10 commented Jan 2, 2025 •

edited

Loading

corona10 commented Jan 2, 2025

colesbury left a comment

gh-128137: Update PyASCIIObject to handle interned field with the atomic operation #128196

gh-128137: Update PyASCIIObject to handle interned field with the atomic operation #128196

Conversation

corona10 commented Dec 23, 2024 • edited by bedevere-app bot Loading

corona10 commented Dec 23, 2024

corona10 commented Dec 23, 2024

colesbury Dec 23, 2024

Choose a reason for hiding this comment

colesbury commented Dec 23, 2024

corona10 commented Dec 23, 2024 • edited Loading

bedevere-bot commented Jan 2, 2025

corona10 commented Jan 2, 2025 • edited Loading

corona10 commented Jan 2, 2025

colesbury left a comment

Choose a reason for hiding this comment

corona10 commented Dec 23, 2024 •

edited by bedevere-app bot

Loading

corona10 commented Dec 23, 2024 •

edited

Loading

corona10 commented Jan 2, 2025 •

edited

Loading