NotificationsYou must be signed in to change notification settings
Fork32.1k
Star67.3k

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

corona10 merged 19 commits intopython:mainfromcorona10:gh-128137

Jan 5, 2025

Merged

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

corona10 merged 19 commits intopython:mainfromcorona10:gh-128137

Jan 5, 2025

Conversation

Copy link

Member

corona10 commentedDec 23, 2024•
edited by bedevere-appbot
Loading

Issue:Race in PyUnicode_InternFromString under free-threading #128137

pythongh-128137: Split out interned field from state

45536d0

corona10 added the skip news label

Dec 23, 2024

corona10 requested review fromvstinner,mpage andcolesbury

December 23, 2024 15:31

bedevere-appbot mentioned this pull request

Dec 23, 2024

Race in PyUnicode_InternFromString under free-threading#128137

Closed

bedevere-appbot added the awaiting core review label

Dec 23, 2024

Copy link

MemberAuthor

corona10 commentedDec 23, 2024

@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.

corona10 added the topic-free-threading label

Dec 23, 2024

kumaraditya303 reviewed

Dec 23, 2024

View reviewed changes

Include/cpython/unicodeobject.h OutdatedShow resolvedHide resolved

colesbury reviewed

Dec 23, 2024

View reviewed changes

Include/cpython/unicodeobject.h OutdatedShow resolvedHide resolved

Address code review

f30b355

corona10 requested a review fromcolesbury

December 23, 2024 15:54

corona10 commented

Dec 23, 2024

View reviewed changes

Include/cpython/unicodeobject.h OutdatedShow resolvedHide resolved

Copy link

MemberAuthor

corona10 commentedDec 23, 2024

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

Maintain unused size as 24 bits

18a8e6b

colesbury reviewed

Dec 23, 2024

View reviewed changes

Include/cpython/unicodeobject.h Outdated

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

Copy link

Contributor

colesburyDec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think this remain in thestate 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 instate will make it easier to keepstate 32-bits due to alignment.

Include/cpython/unicodeobject.h OutdatedShow resolvedHide resolved

Copy link

Contributor

colesbury commentedDec 23, 2024

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

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 added3 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 fromcolesbury

December 23, 2024 16:46

Copy link

MemberAuthor

corona10 commentedDec 23, 2024•
edited
Loading

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.

corona10 added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section label

Jan 2, 2025

Copy link

bedevere-bot commentedJan 2, 2025

🤖 New build scheduled with the buildbot fleet by@corona10 for commit91907aa 🤖

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

bedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section label

Jan 2, 2025

Copy link

MemberAuthor

corona10 commentedJan 2, 2025•
edited
Loading

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

Copy link

MemberAuthor

corona10 commentedJan 2, 2025

In Linux

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

So no regression will be.

colesbury approved these changes

Jan 2, 2025

View reviewed changes

Copy link

Contributor

colesbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

methane approved these changes

Jan 5, 2025

View reviewed changes

corona10 merged commitae23a01 intopython:main

Jan 5, 2025

118 checks passed

bedevere-appbot 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

Closed

clin1234 added a commit to clin1234/pyo3 that referenced this pull request

Jan 21, 2025

Rework PyASCIIObject and PyUnicodeObject to be compatible with 3.14

b90abf3

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

clin1234 added a commit to clin1234/pyo3 that referenced this pull request

Jan 23, 2025

Rework PyASCIIObject and PyUnicodeObject to be compatible with 3.14

1f0d756

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

clin1234 added a commit to clin1234/pyo3 that referenced this pull request

Feb 6, 2025

Rework PyASCIIObject and PyUnicodeObject to be compatible with 3.14

cd272b3

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

clin1234 added a commit to clin1234/pyo3 that referenced this pull request

Feb 13, 2025

Rework PyASCIIObject and PyUnicodeObject to be compatible with 3.14

43d9ffb

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

colesbury mentioned this pull request

Feb 13, 2025

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

Closed

clin1234 added a commit to clin1234/pyo3 that referenced this pull request

Mar 4, 2025

Rework PyASCIIObject and PyUnicodeObject to be compatible with 3.14

a2bc676

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

clin1234 added a commit to clin1234/pyo3 that referenced this pull request

Mar 10, 2025

Rework PyASCIIObject and PyUnicodeObject to be compatible with 3.14

993c14e

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

clin1234 added a commit to clin1234/pyo3 that referenced this pull request

Mar 19, 2025

Rework PyASCIIObject and PyUnicodeObject to be compatible with 3.14

e9b618c

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

clin1234 added a commit to clin1234/pyo3 that referenced this pull request

Mar 20, 2025

Rework PyASCIIObject and PyUnicodeObject to be compatible with 3.14

46b7c5b

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

clin1234 added a commit to clin1234/pyo3 that referenced this pull request

Mar 24, 2025

Rework PyASCIIObject and PyUnicodeObject to be compatible with 3.14

4206a8b

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

clin1234 added a commit to clin1234/pyo3 that referenced this pull request

Mar 31, 2025

Rework PyASCIIObject and PyUnicodeObject to be compatible with 3.14

b2665e1

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

clin1234 added a commit to clin1234/pyo3 that referenced this pull request

Apr 10, 2025

Rework PyASCIIObject and PyUnicodeObject to be compatible with 3.14

e30f997

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

ngoldbaum pushed a commit to clin1234/pyo3 that referenced this pull request

Apr 10, 2025

Rework PyASCIIObject and PyUnicodeObject to be compatible with 3.14

ecbc9b5

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

ngoldbaum pushed a commit to clin1234/pyo3 that referenced this pull request

Apr 11, 2025

Rework PyASCIIObject and PyUnicodeObject to be compatible with 3.14

24a1075

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

clin1234 added a commit to clin1234/pyo3 that referenced this pull request

Apr 17, 2025

Rework PyASCIIObject and PyUnicodeObject to be compatible with 3.14

dfa564f

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

github-merge-queuebot pushed a commit to PyO3/pyo3 that referenced this pull request

Apr 26, 2025

Bump supported cpython version to 3.14 for testing (#4811)

5635b0f

* 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.14Due topython/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 macroWhen `rustc` is invoked, the macro is included with the `--check-cfg`flag, but not with the `--cfg` flag. This caused errors about duplicatedefinitions to spew out when building with stable Rust toolchains.* Revert "Actually add Py_3_14 as a legitimate macro"This reverts commit5da57af.* 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 commit3dd439d.* 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 signaturesNo annotations or explicit default values yetFixes 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>

newcomertv pushed a commit to newcomertv/pyo3 that referenced this pull request

Apr 28, 2025

Bump supported cpython version to 3.14 for testing (PyO3#4811)

9a0b5df

* 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.14Due topython/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 macroWhen `rustc` is invoked, the macro is included with the `--check-cfg`flag, but not with the `--cfg` flag. This caused errors about duplicatedefinitions to spew out when building with stable Rust toolchains.* Revert "Actually add Py_3_14 as a legitimate macro"This reverts commit5da57af.* 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 commit3dd439d.* 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 signaturesNo annotations or explicit default values yetFixes 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>