gh-115999: Add free-threaded specialization for ``TO_BOOL`` by corona10 · Pull Request #126616 · python/cpython

corona10 · 2024-11-09T09:10:48Z

None / bool / int / str are immutable types, so they is thread-safe.
list is mutable, but by using PyList_GET_SIZE we can make it as thread-safe.

Issue: Make the specializing interpreter thread-safe in --disable-gil builds #115999

* None / bool / int / str are immutable types, so they is thread-safe. * list is mutable, but by using ``PyList_GET_SIZE`` we can make it as thread-safe.

eendebakpt · 2024-11-09T11:45:56Z

+ if (PyAnySet_CheckExact(value)) {
+ return SPEC_FAIL_TO_BOOL_SET;
+ }
+ if (PyTuple_CheckExact(value)) {


Maybe not for this PR to address, but why is the tuple type handled here? I see no specialization for tuple (or did I miss something?). Tuple is immutable, so it should be relatively easy to add

IIRC, we added specializaiton based on metric that is measured at https://github.com/faster-cpython/benchmarking-public.
I am not sure how many tuple cases are actually existed.

I agree that tuple case is easy to add but that would be based on how many boolean operations are existed for the tuple at the real world workload.

mpage

Thanks for doing this! Looks good overall. In addition to the comment inline, we're also going to need to update _GUARD_TYPE_VERSION to load tp->tp_version tag using an FT_ATOMIC_LOAD_UINT32_RELAXED.

My review raced with your updates, I hope the comments still make sense.

bedevere-app · 2024-11-10T07:27:50Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

corona10 · 2024-11-16T02:34:34Z

I have made the requested changes; please review again

bedevere-app · 2024-11-16T02:34:39Z

Thanks for making the requested changes!

@markshannon, @mpage: please review the changes made to this pull request.

corona10 · 2024-11-16T02:37:24Z

Mark,
I have addressed your code review especially for #126616 (comment)

I am okay with surrounding #ifdef Py_STATS but it will need extra PR for specialize and unspecialize function, so I prefer to handle it separately, and I believe that @mpage will handle this. (or I will submit the patch for this)

Please take a look :)

mpage

I still don't understand the rationale for keeping the gotos, but I care more about seeing this PR move forward. Accepting to unblock.

corona10 · 2024-11-21T15:09:59Z

@mpage I 've followed the changes from #127030, Would you like to take a look again?

mpage

LGTM!

bedevere-app · 2024-11-21T16:58:04Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

corona10 · 2024-11-21T22:52:06Z

I am merging this PR, @markshannon. Please let me know if we have to handle more things in the future PRs. Matt and I will try to adjust what you concerns :) cc @mpage

…thongh-126616)

pythongh-115999: Add free-threaded specialization forTO_BOOL
62b0307
* None / bool / int / str are immutable types, so they is thread-safe. * list is mutable, but by using ``PyList_GET_SIZE`` we can make it as thread-safe.

corona10 requested a review from markshannon as a code owner November 9, 2024 09:10

bedevere-appBot added the awaiting core review label Nov 9, 2024

corona10 requested a review from mpage November 9, 2024 09:10

bedevere-appBot mentioned this pull request Nov 9, 2024
Make the specializing interpreter thread-safe in --disable-gil builds #115999
Closed

corona10 added the skip news label Nov 9, 2024

nit
e32f2f0

corona10 marked this pull request as draft November 9, 2024 09:44

bedevere-appBot removed the awaiting core review label Nov 9, 2024

corona10 commented Nov 9, 2024
View reviewed changes

Comment threadPython/specialize.c Outdated

Make set cache as atomic operation
6798366

corona10 marked this pull request as ready for review November 9, 2024 10:33

bedevere-appBot added the awaiting core review label Nov 9, 2024

fix
e45778b

corona10 requested a review from Fidget-Spinner November 9, 2024 10:53

corona10 changed the title ~~gh-115999: Add free-threaded specialization for TO_BOOL~~[WIP] gh-115999: Add free-threaded specialization for TO_BOOLNov 9, 2024

corona10 commented Nov 9, 2024
View reviewed changes

Comment threadPython/specialize.c Outdated

fix
bafabd7

corona10 changed the title ~~[WIP] gh-115999: Add free-threaded specialization for TO_BOOL~~gh-115999: Add free-threaded specialization for TO_BOOLNov 9, 2024

corona10 added the topic-free-threading label Nov 9, 2024

eendebakpt reviewed Nov 9, 2024
View reviewed changes

corona10and others added 2 commits November 9, 2024 21:00

nit
aa8a81c

Address code review
1f4a350

mpage requested changes Nov 10, 2024
View reviewed changes

Comment threadPython/specialize.c Outdated
Comment threadPython/specialize.c Outdated

bedevere-appBot removed the awaiting core review label Nov 10, 2024

bedevere-appBot added the awaiting changes label Nov 10, 2024

Address code review
23e190e

corona10 requested a review from mpage November 10, 2024 08:26

This comment was marked as spam.
Sign in to view

mpage requested a review from markshannon November 14, 2024 03:23

corona10 added 3 commits November 16, 2024 11:18

Merge remote-tracking branch 'upstream/main' intopythongh-115999-bool
a213390

Address code review
e68dc65

Address code review
b27f916

bedevere-appBot added awaiting change review and removed awaiting changes labels Nov 16, 2024

bedevere-appBot requested a review from mpage November 16, 2024 02:34

nit
249efd1

mpage approved these changes Nov 16, 2024
View reviewed changes

bedevere-appBot added awaiting merge and removed awaiting change review labels Nov 16, 2024

corona10 added 2 commits November 22, 2024 00:04

Merge remote-tracking branch 'upstream/main' intopythongh-115999-bool
c08f8b7

Address code review
6bc4c90

Use TO_BOOL for SPECIALIZATION_FAIL
542e4dd

mpage requested changes Nov 21, 2024
View reviewed changes

bedevere-appBot removed the awaiting merge label Nov 21, 2024

bedevere-appBot added the awaiting changes label Nov 21, 2024

mpage approved these changes Nov 21, 2024
View reviewed changes

bedevere-appBot added awaiting merge and removed awaiting changes labels Nov 21, 2024

corona10 merged commit 78a530a into python:mainNov 21, 2024

bedevere-appBot removed the awaiting merge label Nov 21, 2024

corona10 deleted the gh-115999-bool branch November 21, 2024 22:52

ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
pythongh-115999: Add free-threaded specialization forTO_BOOL(py…
0da364f
…thongh-126616)

Uh oh!

Uh oh!

Conversation

corona10 commented Nov 9, 2024• edited by bedevere-app BotLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eendebakptNov 9, 2024

Choose a reason for hiding this comment

Uh oh!

corona10Nov 9, 2024

Choose a reason for hiding this comment

Uh oh!

corona10Nov 9, 2024

Choose a reason for hiding this comment

Uh oh!

mpage left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bedevere-appBot commented Nov 10, 2024

Uh oh!

This comment was marked as spam.

Uh oh!

corona10 commented Nov 16, 2024

Uh oh!

bedevere-appBot commented Nov 16, 2024

Uh oh!

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

Uh oh!

Uh oh!

mpage left a comment

Choose a reason for hiding this comment

Uh oh!

corona10 commented Nov 21, 2024

Uh oh!

mpage left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-appBot commented Nov 21, 2024

Uh oh!

corona10 commented Nov 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

corona10 commented Nov 9, 2024•
edited by bedevere-app Bot
Loading

mpage left a comment •
edited
Loading

corona10 commented Nov 16, 2024•
edited
Loading