GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-128137: Update PyASCIIObject to handle interned field with the atomic operation by corona10 · Pull Request #128196 · python/cpython

Conversation

corona10 commented Dec 23, 2024 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Issue: Race in PyUnicode_InternFromString under free-threading #128137

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          pythongh-128137: Split out interned field from state

45536d0

corona10 added the skip news label

Dec 23, 2024

corona10 requested review from colesbury, mpage and vstinner

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 commented Dec 23, 2024

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

corona10 added the topic-free-threading label

Dec 23, 2024

kumaraditya303 reviewed

Dec 23, 2024

View reviewed changes

Comment thread

Include/cpython/unicodeobject.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

colesbury reviewed

Dec 23, 2024

View reviewed changes

Comment thread

Include/cpython/unicodeobject.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Include/cpython/unicodeobject.h Resolved

Uh oh!

There was an error while loading. Please reload this page.


          Address code review

f30b355

corona10 requested a review from colesbury

December 23, 2024 15:54

corona10 commented

Dec 23, 2024

View reviewed changes

Comment thread

Include/cpython/unicodeobject.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

corona10 commented Dec 23, 2024

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Maintain unused size as 24 bits

18a8e6b

colesbury reviewed

Dec 23, 2024

View reviewed changes

Comment thread

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;

colesbury Dec 23, 2024

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

corona10 reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

Include/cpython/unicodeobject.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

colesbury commented Dec 23, 2024

Copy link

Copy Markdown

Contributor

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

corona10 reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          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 commented Dec 23, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member Author

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.

encukou reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Jan 2, 2025

bedevere-bot commented Jan 2, 2025

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Jan 2, 2025

corona10 commented Jan 2, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

corona10 commented Jan 2, 2025

Copy link

Copy Markdown

Member Author

In Linux

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

So no regression will be.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

colesbury approved these changes

Jan 2, 2025

View reviewed changes

colesbury left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this 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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

methane approved these changes

Jan 5, 2025

View reviewed changes

corona10 merged commit ae23a01 into python:main

Jan 5, 2025

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

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 to python/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 to python/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 to python/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 to python/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 in intern_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 to python/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 to python/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 to python/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 to python/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 to python/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 to python/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 to python/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 to python/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 to python/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 to python/cpython#128196, data types within `PyASCIIObject.state` have changed, resulting in test failures when building against 3.14.

github-merge-queue Bot 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.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 <[email protected]>
Co-authored-by: David Hewitt <[email protected]>
Co-authored-by: Thomas Tanon <[email protected]>

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.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 <[email protected]>
Co-authored-by: David Hewitt <[email protected]>
Co-authored-by: Thomas Tanon <[email protected]>

Reviewers

kumaraditya303

kumaraditya303 left review comments

methane

methane approved these changes

colesbury

colesbury approved these changes

vstinner

Awaiting requested review from vstinner

mpage

Awaiting requested review from mpage

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

topic-free-threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

corona10 commented Dec 23, 2024 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corona10 commented Dec 23, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

corona10 commented Dec 23, 2024

Uh oh!

colesbury Dec 23, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

colesbury commented Dec 23, 2024

Uh oh!

corona10 commented Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Jan 2, 2025

Uh oh!

corona10 commented Jan 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corona10 commented Jan 2, 2025

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

corona10 commented Dec 23, 2024 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

corona10 commented Dec 23, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

corona10 commented Jan 2, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.