bpo-42927: Inline cache for slots#24216

gvanrossum

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

vstinner

cc @methane @pablogsal

pablogsal

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

gvanrossum

Okay, assuming the tests pass, this is the version I'm sending for review.

gvanrossum

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

pablogsal

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.

bedevere-bot

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

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

pablogsal

LGTM! Let's do a buildbot pass to make sure there are no reference leaks.

pablogsal

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

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.

The problem was that sometimes a hint of -1 could be set, meaning the dict lookup failed. My code mistook that for a slot offset. The fix is simple, because slot offsets can never be 0, so ~offset can never be -1. I also cleaned up some comments and a variable name.

gvanrossum

Here's the fix. @pablogsal how do I trigger a buildbot run? I can't find that in the devguide.

pablogsal

Here's the fix. @pablogsal how do I trigger a buildbot run? I can't find that in the devguide.

You can add the "run with buildbots" label to the PR and the checks should start appearing at the bottom together with Travis and the other CI.

bedevere-bot

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

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

bedevere-bot

🤖 New build scheduled with the buildbot fleet by @gvanrossum for commit 17c8a95 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

gvanrossum

Looks like only 2 buildbots failed.

AMD64 FreeBSD Shared PR: Crash in test_gdb with Exception: Command 'gdb -nx --version' failed with exit code 1: stdout='' stderr='ld-elf.so.1: Shared object "libncursesw.so.6" not found, required by "gdb"\n'; also failing are test_subprocess (another libcurses issue) and test_venv (test_deactivate_with_strict_bash_opts fails with a subprocess error).
s390x Fedora Refleaks PR: test_curses failed in test_init_pair. Do we know if that passes for master?

This seems a better result than before.

gvanrossum

I realized there's another optimization for this case. The code always starts with PyObject *name = GETITEM(names, oparg); which calls PyTuple_GetItem(). But if the fast slot/field path is taken we never need that! However the fast dict hint path does need it, and of course all slow paths need it. So the logic for deciding to get this would be convoluted (it would have to be in various else branches).

Is that worth exploring? It might be hard to prove there's no slowdown for the paths that do need the name. I could put this off to another PR -- if slots become more popular because of this PR we can add it later. :-)

gvanrossum

I’m just going to land this if there are no objections by tonight.

bedevere-bot

@gvanrossum: Please replace # with GH- in the commit message next time. Thanks!

…on#24216)

Implement inline cache for slots

aca2939

gvanrossum requested a review from markshannon as a code owner January 14, 2021 04:54

the-knights-who-say-ni added the CLA signed label Jan 14, 2021

bedevere-bot added the awaiting core review label Jan 14, 2021

1st1 reviewed Jan 14, 2021

View reviewed changes

serhiy-storchaka self-requested a review January 14, 2021 09:14

pablogsal reviewed Jan 14, 2021

View reviewed changes

gvanrossum and others added 3 commits January 15, 2021 10:51

Comment out debug print

e334e53

Fall through to slow path if slot is NULL

ec0a590

📜🤖 Added by blurb_it.

b725a65

rhettinger approved these changes Jan 21, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Jan 21, 2021

pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 21, 2021

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 21, 2021

pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 28, 2021

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 28, 2021

Merge remote-tracking branch 'origin/master' into slotscache

17c8a95

gvanrossum added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 28, 2021

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 28, 2021

1st1 reviewed Jan 28, 2021

View reviewed changes

Improve comment

86c4b39

1st1 approved these changes Jan 29, 2021

View reviewed changes

gvanrossum merged commit 5c5a938 into python:master Jan 30, 2021

bedevere-bot removed the awaiting merge label Jan 30, 2021

gvanrossum deleted the slotscache branch January 30, 2021 02:02

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021

bpo-42927: Inline cache for attributes defined with '__slots__' (pyth… …

89abb84

…on#24216)

Conversation

gvanrossum commented Jan 14, 2021 • edited by pablogsal Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Jan 14, 2021

Uh oh!

pablogsal commented Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum commented Jan 15, 2021

Uh oh!

gvanrossum commented Jan 15, 2021

Uh oh!

pablogsal commented Jan 15, 2021

Uh oh!

bedevere-bot commented Jan 21, 2021

Uh oh!

pablogsal commented Jan 21, 2021

Uh oh!

pablogsal commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum commented Jan 22, 2021

Uh oh!

gvanrossum commented Jan 28, 2021

Uh oh!

pablogsal commented Jan 28, 2021

Uh oh!

bedevere-bot commented Jan 28, 2021

Uh oh!

bedevere-bot commented Jan 28, 2021

Uh oh!

gvanrossum commented Jan 29, 2021

Uh oh!

gvanrossum commented Jan 29, 2021

Uh oh!

gvanrossum commented Jan 29, 2021 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Jan 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

gvanrossum commented Jan 14, 2021 •

edited by pablogsal

Loading

pablogsal commented Jan 14, 2021 •

edited

Loading

pablogsal commented Jan 21, 2021 •

edited

Loading

gvanrossum commented Jan 29, 2021 via email •

edited

Loading