Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

corona10
Copy link
Member

@corona10 corona10 commented Dec 23, 2024

@corona10
Copy link
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.

Include/cpython/unicodeobject.h Outdated Show resolved Hide resolved
Include/cpython/unicodeobject.h Outdated Show resolved Hide resolved
@corona10 corona10 requested a review from colesbury December 23, 2024 15:54
@corona10
Copy link
Member Author

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

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

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.

Include/cpython/unicodeobject.h Outdated Show resolved Hide resolved
@colesbury
Copy link
Contributor

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

@corona10 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 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 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 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 corona10 requested a review from colesbury December 23, 2024 16:46
@corona10
Copy link
Member Author

corona10 commented Dec 23, 2024

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.

Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

LGTM, assuming the tests pass

@colesbury
Copy link
Contributor

I agree -- I don't think we should backport this.

@corona10
Copy link
Member Author

There looks like issue with Win32, I will see tomorrow :)

@vstinner
Copy link
Member

cc @methane

@corona10 corona10 changed the title gh-128137: Update PyASCIIObject to handle interned field with the atomic operation [WIP] gh-128137: Update PyASCIIObject to handle interned field with the atomic operation Dec 24, 2024
@corona10 corona10 force-pushed the gh-128137 branch 4 times, most recently from 610896e to b1b396b Compare December 24, 2024 09:27
@corona10
Copy link
Member Author

I am now debugging on my Windows machine..

@corona10 corona10 changed the title [WIP] gh-128137: Update PyASCIIObject to handle interned field with the atomic operation gh-128137: Update PyASCIIObject to handle interned field with the atomic operation Dec 24, 2024
@corona10
Copy link
Member Author

@colesbury @methane

I 've updated test_str.py and test_sys.py for win32 and WASI, but there seems to be an alignment issue that we didn't care about before.
Do you think the current implementation is fine, or would you prefer to redesign the layout to be more consistent?
(Not sure about this currently)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants