Skip to content

[mypyc] Access tuple attributes under critical section - #22226

Open
p-sawicki wants to merge 4 commits into
python:masterfrom
p-sawicki:tuple-access-critical-section
Open

p-sawicki wants to merge 4 commits into
python:masterfrom
p-sawicki:tuple-access-critical-section

Conversation

@p-sawicki

Copy link
Copy Markdown
Collaborator

Work on mypyc/mypyc#1203

Take a critical section when accessing fixed-length tuple attributes on free-threaded builds. Fields of these tuples are stored in a C struct so concurrent updates could result in readers observing a state in the middle of the update. Also use atomic operations on bitmaps on free-threaded builds.

Final tuple attributes are unaffected. I saw pretty much no difference in mypy self-check benchmark on 3.14t.

@JukkaL JukkaL left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good overall, I just had some style nits.

Comment thread mypyc/codegen/emit.py Outdated
check = f"{check} && !({bitmap} & {bit})"
self.emit_line(f"if ({check}) {{")

def emit_tuple_attr_load(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It would be good for the name to make it clear that this a synchronized load.

Comment thread mypyc/codegen/emit.py Outdated
self.emit_line("Py_END_CRITICAL_SECTION();")
return undefined

def emit_tuple_attr_store(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Similar to above, update name?

Comment thread mypyc/codegen/emit.py Outdated
delete: bool = False,
is_init: bool = False,
) -> None:
"""Publish a tuple under the owner lock, releasing old references outside it."""

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The above description is a bit odd: "Publish", "releasing old references outside it". Can you make it clearer?

Comment thread mypyc/test/test_emitfunc.py Outdated
""",
)

def test_final_tuple_attr_reads(self) -> None:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This test case seems to roughly replicate the emitted logic, which seems kind of pointless. If there is some important case that should be covered, I think it's better to have an unconditional emit. Not every combination needs to be covered here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

ok i've deleted this one and added a simpler one to check that a final attribute doesn't take the lock on free-threaded builds.

Comment thread mypyc/codegen/emit.py Outdated
is_init: bool = False,
) -> None:
"""Publish a tuple under the owner lock, releasing old references outside it."""
"""Assign a tuple attribute attr to src under the owner lock.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This seems reversed -- should be assign src to a tuple attribute?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants