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-115999: Add free-threaded specialization for STORE_SUBSCR #127169

Merged
merged 2 commits into from
Nov 26, 2024

Conversation

colesbury
Copy link
Contributor

@colesbury colesbury commented Nov 22, 2024

The specialization only depends on the type, so no special thread-safety considerations there.

STORE_SUBSCR_LIST_INT needs to lock the list before modifying it.

_PyDict_SetItem_Take2 already internally locks the dictionary using a critical section.

The specialization only depends on the type, so no special thread-safety
considerations there.

STORE_SUBSCR_LIST_INT needs to lock the list before modifying it.

`_PyDict_SetItem_Take2` already internally locks the dictionary using a
critical section.
// avoid any potentially escaping calls (like PyStackRef_CLOSE) while the
// object is locked.
#ifdef Py_GIL_DISABLED
# define LOCK_OBJECT(op) PyMutex_LockFast(&(_PyObject_CAST(op))->ob_mutex)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mpage, I think this locking pattern will be slightly faster than using critical sections inline in bytecodes.c. The downside is that we will deopt if there's any lock contention, but I think that's an okay tradeoff.

If we decide to go with this pattern, we can update UNPACK_SEQUENCE_LIST to use it.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense to me.

@colesbury colesbury requested a review from corona10 November 22, 2024 19:18
@colesbury
Copy link
Contributor Author

@corona10 - I put my name down next to STORE_ATTR, but then got confused and worked on STORE_SUBSCR instead. Sorry!

@colesbury
Copy link
Contributor Author

(STORE_ATTR is up for grabs)

Copy link
Contributor

@mpage mpage left a comment

Choose a reason for hiding this comment

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

Nice! I left one comment/question inline and there's a compiler warning that we need to fix up.

// avoid any potentially escaping calls (like PyStackRef_CLOSE) while the
// object is locked.
#ifdef Py_GIL_DISABLED
# define LOCK_OBJECT(op) PyMutex_LockFast(&(_PyObject_CAST(op))->ob_mutex)
Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense to me.

Python/specialize.c Show resolved Hide resolved
@corona10
Copy link
Member

I put my name down next to STORE_ATTR, but then got confused and worked on STORE_SUBSCR instead. Sorry!

Race condition with the wrong lock :) Fine, thank you for the work, I've updated the list of issues.

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

Please add test codes at test_opcache.py

class TestSpecializer(TestBase):

Python/ceval_macros.h Outdated Show resolved Hide resolved
Co-authored-by: Donghee Na <donghee.na92@gmail.com>
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

lgtm (except test code :))

@colesbury
Copy link
Contributor Author

Performance difference is marginal: 1.00x faster (but 99.99% likely to be faster)

https://github.com/facebookexperimental/free-threading-benchmarking/blob/main/results/bm-20241122-3.14.0a2%2B-2b63929-NOGIL/bm-20241122-vultr-x86_64-colesbury-gh_115999_store_subs-3.14.0a2%2B-2b63929-vs-base.md

I think it's worth doing even if the perf is neutral just to minimize divergence with the default (GIL) build.

@colesbury colesbury merged commit 71ede11 into python:main Nov 26, 2024
62 checks passed
@colesbury colesbury deleted the gh-115999-store-subscr branch November 26, 2024 21:46
mdboom added a commit that referenced this pull request Dec 2, 2024
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 8, 2025
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
…ython#127169)

The specialization only depends on the type, so no special thread-safety
considerations there.

STORE_SUBSCR_LIST_INT needs to lock the list before modifying it.

`_PyDict_SetItem_Take2` already internally locks the dictionary using a
critical section.
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
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.

3 participants