Sitelet https://web.archive.org/web/20210126132200/https://github.com/python/cpython/pull/24216
Skip to content

/ cpython

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

Jump to bottom

bpo-42927: Inline cache for slots #24216

Open
wants to merge 4 commits into
base: master
from
Open

bpo-42927: Inline cache for slots #24216

wants to merge 4 commits into from
+112 −61

Conversation

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Jan 14, 2021 •
edited by pablogsal

This is a modification of the LOAD_ATTR inline cache that landed a few months ago (PR 22803).

The design overloads the hint field: if it is negative, it indicates a slot. Slots are created using __slots__, but some builtin types that use PyMemberDescrObject descriptors (with the type set to T_OBJECT_EX) will also benefit.

I've tried to keep the code style similar to the existing LOAD_ATTR cache (happy path first) but I had to make some compromises, e.g. the cache initialization code can no longer skip everything if tp_dictoffset is zero (since it will be zero for classes with only slots).

https://bugs.python.org/issue42927
@gvanrossum
Implement inline cache for slots
Loading status checks…
aca2939
@gvanrossum gvanrossum requested a review from markshannon as a code owner Jan 14, 2021
@1st1
1st1 reviewed Jan 14, 2021
View changes
Python/ceval.c Outdated Show resolved Hide resolved
@serhiy-storchaka serhiy-storchaka self-requested a review Jan 14, 2021
@vstinner
Copy link
Member

@vstinner vstinner commented Jan 14, 2021

cc @methane @pablogsal
@pablogsal
pablogsal reviewed Jan 14, 2021
View changes
Python/ceval.c Outdated Show resolved Hide resolved
@pablogsal
Copy link
Member

@pablogsal pablogsal commented Jan 14, 2021 •
edited

Something is wrong, we are reaching ther error label in the eval loop without an error set (check the CI). (The cache is not active in debug mode so if you want to reproduce compile without --with-pydebug). See this comment: https://github.com/python/cpython/pull/24216/files#r557650392
@pablogsal
pablogsal reviewed Jan 14, 2021
View changes
Python/ceval.c Outdated Show resolved Hide resolved
@pablogsal
pablogsal reviewed Jan 14, 2021
View changes
Python/ceval.c Outdated Show resolved Hide resolved
gvanrossum and others added 3 commits Jan 15, 2021
@gvanrossum
Comment out debug print
Loading status checks…
e334e53
@gvanrossum
Fall through to slow path if slot is NULL
Loading status checks…
ec0a590
@blurb-it
📜🤖 Added by blurb_it.
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
b725a65
@gvanrossum
Copy link
Member Author

@gvanrossum gvanrossum commented Jan 15, 2021

Okay, assuming the tests pass, this is the version I'm sending for review.
@gvanrossum
Copy link
Member Author

@gvanrossum gvanrossum commented Jan 15, 2021

It seems test_ssl on macOS failed, but the other test failures have been resolved. Is that test flaky?
@pablogsal
Copy link
Member

@pablogsal pablogsal commented Jan 15, 2021

It seems test_ssl on macOS failed, but the other test failures have been resolved. Is that test flaky?

Given the nature of this change and the fact that the error says:

ConnectionRefusedError: [Errno 61] Connection refused

I would say is unrelated. I have restarted manually the tests.
@rhettinger
rhettinger approved these changes Jan 21, 2021
View changes
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jan 21, 2021

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit b725a65 🤖

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.
@pablogsal
Copy link
Member

@pablogsal pablogsal commented Jan 21, 2021

LGTM! Let's do a buildbot pass to make sure there are no reference leaks.
@pablogsal
Copy link
Member

@pablogsal pablogsal commented Jan 21, 2021 •
edited

Seems we are missing something because some of the buildbots are crashing with segmentation faults. For instance:

https://buildbot.python.org/all/#/builders/459/builds/69

test_signature_on_decorated (test.test_inspect.TestSignatureObject) ... ok
Fatal Python error: Segmentation fault
Current thread 0x00007fff99164920 (most recent call first):
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/inspect.py", line 2304 in _signature_from_callable
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/inspect.py", line 2877 in from_callable
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/inspect.py", line 3129 in signature
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/test/test_inspect.py", line 2392 in test_signature_on_decorated_builtins
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/unittest/case.py", line 549 in _callTestMethod
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/unittest/case.py", line 592 in run
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/unittest/case.py", line 652 in __call__
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/unittest/suite.py", line 122 in run
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/unittest/suite.py", line 84 in __call__
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/unittest/suite.py", line 122 in run
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/unittest/suite.py", line 84 in __call__
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/unittest/runner.py", line 176 in run
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/test/support/__init__.py", line 959 in _run_suite
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/test/support/__init__.py", line 1082 in run_unittest
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/test/test_inspect.py", line 4062 in test_main
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/test/libregrtest/runtest.py", line 236 in _runtest_inner2
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/test/libregrtest/runtest.py", line 272 in _runtest_inner
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/test/libregrtest/runtest.py", line 155 in _runtest
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/test/libregrtest/runtest.py", line 195 in runtest
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/test/libregrtest/main.py", line 319 in rerun_failed_tests
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/test/libregrtest/main.py", line 696 in _main
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/test/libregrtest/main.py", line 639 in main
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/test/libregrtest/main.py", line 717 in main
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/test/__main__.py", line 2 in <module>
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/runpy.py", line 87 in _run_code
  File "/home/buildbot/buildarea/pull_request.cstratak-RHEL8-ppc64le.lto-pgo/build/Lib/runpy.py", line 197 in _run_module_as_main
Extension modules: _testcapi (total: 1)
make: *** [Makefile:1228: buildbottest] Segmentation fault (core dumped)
program finished with exit code 2
elapsedTime=259.101476

The failures seem to be in test_inspect and test_zipfile
@gvanrossum
Copy link
Member Author

@gvanrossum gvanrossum commented Jan 22, 2021

Hm. I think this may happen when _PyDict_GetItemHint() returns -1. Then we set la->hint = -1 and then the next time we get here it is interpreted as a slot hint. I'll come up with a fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@1st1 1st1

@pablogsal pablogsal

@rhettinger rhettinger

@serhiy-storchaka serhiy-storchaka

@markshannon markshannon

Assignees
No one assigned
Labels
CLA signed awaiting merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@gvanrossum @vstinner @pablogsal @bedevere-bot @1st1 @rhettinger @the-knights-who-say-ni