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: Enable BINARY_SUBSCR_GETITEM for free-threaded build #127737

Merged
merged 18 commits into from
Dec 19, 2024

Conversation

corona10
Copy link
Member

@corona10 corona10 commented Dec 8, 2024

@corona10 corona10 changed the title [WIP] gh-115999: Enable BINARY_SUBSCR_GETITEM for free-threaded build gh-115999: Enable BINARY_SUBSCR_GETITEM for free-threaded build Dec 8, 2024
@corona10 corona10 marked this pull request as ready for review December 8, 2024 11:06
@corona10 corona10 requested a review from markshannon as a code owner December 8, 2024 11:06
@corona10
Copy link
Member Author

corona10 commented Dec 8, 2024

@mpage, @colesbury

To enable BINARY_SUBSCR_GETITEM, we should handle TypeObject and _spec_cache which are mutable by other threads.
To handle this, I have introduced PyType_GetItemFromCache, _PyType_GetItemFromCacheWithVersion, and _PyType_CacheGetItemForSpecialization to lock the type and mutate them at the same transaction.

But I am not sure this approach is proper approach since I noticed that cached items can be mutated before the interpreter gets cached objects, so I have added the logic to check whether the item is NULL or FunctionObject, but please let me know if my approach is wrong.

Anyway, this is the first working version without error. PTAL.

@corona10 corona10 requested review from mpage and colesbury December 8, 2024 11:06
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.

Thanks for working on this! I left a few comments inline.

Objects/typeobject.c Outdated Show resolved Hide resolved
Objects/typeobject.c Outdated Show resolved Hide resolved
Objects/typeobject.c Outdated Show resolved Hide resolved
Objects/typeobject.c Outdated Show resolved Hide resolved
Objects/typeobject.c Outdated Show resolved Hide resolved
@@ -881,8 +880,8 @@ dummy_func(

op(_BINARY_SUBSCR_INIT_CALL, (container, sub -- new_frame: _PyInterpreterFrame* )) {
PyTypeObject *tp = Py_TYPE(PyStackRef_AsPyObjectBorrow(container));
PyHeapTypeObject *ht = (PyHeapTypeObject *)tp;
PyObject *getitem = ht->_spec_cache.getitem;
PyObject *getitem = _PyType_GetItemFromCache(tp);
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 it's probably safe to reload getitem from the cache after the guard passes. (If the getitem in the cache is replaced after the guard passes, the new getitem cannot be mutated in a way that would change the function version: that would require stopping the world). However, I'd prefer that we pass the getitem that is loaded in _BINARY_SUBSCR_CHECK_FUNC to _BINARY_SUBSCR_INIT_CALL.

Python/specialize.c Outdated Show resolved Hide resolved
@corona10 corona10 changed the title gh-115999: Enable BINARY_SUBSCR_GETITEM for free-threaded build [WIP] gh-115999: Enable BINARY_SUBSCR_GETITEM for free-threaded build Dec 15, 2024
@corona10 corona10 changed the title [WIP] gh-115999: Enable BINARY_SUBSCR_GETITEM for free-threaded build gh-115999: Enable BINARY_SUBSCR_GETITEM for free-threaded build Dec 16, 2024
@corona10
Copy link
Member Author

corona10 commented Dec 16, 2024

@mpage Hmm would you like to take a look again?

@corona10 corona10 requested a review from mpage December 16, 2024 15:57
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.

Overall approach is looking good! I'd still prefer it if we passed the getitem that is loaded in _BINARY_SUBSCR_CHECK_FUNC to _BINARY_SUBSCR_INIT_CALL. That would also allow us to avoid introducing the DEOPT_IF in _BINARY_SUBSCR_INIT_CALL. Is there a reason you don't want to do that?

Lib/test/test_opcache.py Outdated Show resolved Hide resolved
Python/bytecodes.c Outdated Show resolved Hide resolved
Python/bytecodes.c Outdated Show resolved Hide resolved
@mpage
Copy link
Contributor

mpage commented Dec 18, 2024

I can't see it. You may forget to press the comment button, I guess :)

Oops - fixed!

@corona10 corona10 requested a review from mpage December 18, 2024 03:26
@corona10
Copy link
Member Author

Done :)

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.

Looks good! I noticed one last thing then I think we're good to go.

We need to update the opcode definition of _BINARY_SUBSCR_INIT_CALL in Python/optimizer_bytecodes.c to handle the extra output (getitem) of _BINARY_SUBSCR_CHECK_FUNC.

@corona10 corona10 requested a review from mpage December 18, 2024 18:47
@corona10
Copy link
Member Author

Done! Even if I am not sure that this change is the correct for tier 2 because I have never handled this before :)

@Fidget-Spinner
Copy link
Member

The tier2 change is correct. I should add an automatic script that checks tier2 and tier1 definitions are synced so that you don't trip over this. Thanks Matt for catching that!

@corona10
Copy link
Member Author

@mpage Okay to merge this PR?

@mpage
Copy link
Contributor

mpage commented Dec 19, 2024

I kicked off a couple of benchmark runs earlier today. They should be done shortly. Let’s wait for the results. Otherwise, looks great!

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.

Looks good! Benchmark results:

  • ~1% improvement on free-threaded builds
  • ~neutral on default builds

@corona10 corona10 merged commit 48c70b8 into python:main Dec 19, 2024
59 checks passed
@corona10 corona10 deleted the gh-115999-BINARY_SUBSCR-final branch December 19, 2024 02:08
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot PPC64LE Fedora Stable 3.x has failed when building commit 48c70b8.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/90/builds/5364) and take a look at the build logs.
  4. Check if the failure is related to this commit (48c70b8) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/90/builds/5364

Failed tests:

  • test_socket

Failed subtests:

  • testStream - test.test_socket.ThreadedVSOCKSocketStreamTest.testStream

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-ppc64le/build/Lib/test/test_socket.py", line 550, in testStream
    msg = self.conn.recv(1024)
PermissionError: [Errno 13] Permission denied

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot PPC64LE Fedora Stable Clang 3.x has failed when building commit 48c70b8.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/435/builds/5142) and take a look at the build logs.
  4. Check if the failure is related to this commit (48c70b8) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/435/builds/5142

Failed tests:

  • test_socket

Failed subtests:

  • testStream - test.test_socket.ThreadedVSOCKSocketStreamTest.testStream

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-ppc64le.clang/build/Lib/test/test_socket.py", line 550, in testStream
    msg = self.conn.recv(1024)
PermissionError: [Errno 13] Permission denied

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot PPC64LE Fedora Stable Clang Installed 3.x has failed when building commit 48c70b8.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/451/builds/5124) and take a look at the build logs.
  4. Check if the failure is related to this commit (48c70b8) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/451/builds/5124

Failed tests:

  • test_socket

Failed subtests:

  • testStream - test.test_socket.ThreadedVSOCKSocketStreamTest.testStream

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-ppc64le.clang-installed/build/target/lib/python3.14/test/test_socket.py", line 550, in testStream
    msg = self.conn.recv(1024)
PermissionError: [Errno 13] Permission denied

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot PPC64LE Fedora Stable LTO + PGO 3.x has failed when building commit 48c70b8.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/101/builds/5356) and take a look at the build logs.
  4. Check if the failure is related to this commit (48c70b8) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/101/builds/5356

Failed tests:

  • test_socket

Failed subtests:

  • testStream - test.test_socket.ThreadedVSOCKSocketStreamTest.testStream

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-ppc64le.lto-pgo/build/Lib/test/test_socket.py", line 550, in testStream
    msg = self.conn.recv(1024)
PermissionError: [Errno 13] Permission denied

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot PPC64LE Fedora Stable LTO 3.x has failed when building commit 48c70b8.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/541/builds/4746) and take a look at the build logs.
  4. Check if the failure is related to this commit (48c70b8) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/541/builds/4746

Failed tests:

  • test_socket

Failed subtests:

  • testStream - test.test_socket.ThreadedVSOCKSocketStreamTest.testStream

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-ppc64le.lto/build/Lib/test/test_socket.py", line 550, in testStream
    msg = self.conn.recv(1024)
PermissionError: [Errno 13] Permission denied

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot PPC64LE Fedora Stable Refleaks 3.x has failed when building commit 48c70b8.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/280/builds/1480) and take a look at the build logs.
  4. Check if the failure is related to this commit (48c70b8) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/280/builds/1480

Failed tests:

  • test_socket

Failed subtests:

  • testStream - test.test_socket.ThreadedVSOCKSocketStreamTest.testStream

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-ppc64le.refleak/build/Lib/test/test_socket.py", line 550, in testStream
    msg = self.conn.recv(1024)
PermissionError: [Errno 13] Permission denied

@markshannon
Copy link
Member

Looks good! Benchmark results:

  • ~1% improvement on free-threaded builds
  • ~neutral on default builds

I don't think a 0.3-0.5% slower on average with a 13% slowdown on the gc_traversal benchmark is "neutral".
It might be just noise, but the large slowdown on the gc_traversal benchmark suggests a real issue.

Could we revert this PR until we have a better understanding of what is going on here?

@colesbury
Copy link
Contributor

The gc_traversal benchmark results sure looks like noise to me. That benchmark is dominated by list_traverse and functions in the GC, none of which have changed.

How about you run the benchmarks in the faster-cpython infrastructure and see if you get the same results?

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