bpo-42927: Inline cache for slots #24216
Conversation
|
Something is wrong, we are reaching ther |
|
Okay, assuming the tests pass, this is the version I'm sending for review. |
|
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:
I would say is unrelated. I have restarted manually the tests. |
pablogsal
added
the
🔨 test-with-buildbots
|
🤖 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. |
bedevere-bot
removed
the
🔨 test-with-buildbots
|
LGTM! Let's do a buildbot pass to make sure there are no reference leaks. |
|
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 The failures seem to be in test_inspect and test_zipfile |
|
Hm. I think this may happen when |
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.
|
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. |
pablogsal
added
the
🔨 test-with-buildbots
|
🤖 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
removed
the
🔨 test-with-buildbots
gvanrossum
added
the
🔨 test-with-buildbots
|
🤖 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. |
bedevere-bot
removed
the
🔨 test-with-buildbots
| // Hint >= 0 is a dict index; hint < 0 is an inverted slot index. | ||
| if (la->hint < 0) { | ||
| /* Even faster path -- slot hint */ | ||
| // Hint >= 0 is a dict index; hint == -1 is a dict miss. |
There was a problem hiding this comment.
Oh, right! How did I miss that on review :/
There was a problem hiding this comment.
Same way I missed it writing the code. :-(
This code is hairy.
Python/ceval.c
Outdated
| if (la->hint < 0) { | ||
| /* Even faster path -- slot hint */ | ||
| // Hint >= 0 is a dict index; hint == -1 is a dict miss. | ||
| // Hint < -1 is an inverted slot offset (offsets are never 0). |
There was a problem hiding this comment.
Maybe clarify that slot offsets are always greater than 1?
There was a problem hiding this comment.
I have a comment ready to push:
// Hint >= 0 is a dict index; hint == -1 is a dict miss.
// Hint < -1 is an inverted slot offset: offset is strictly > 0,
// so ~offset is strictly < -1 (assuming 2's complement).
Let me know if you still want improvement.
|
Looks like only 2 buildbots failed.
This seems a better result than before. |
|
I realized there's another optimization for this case. The code always starts with 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. :-) |
|
I’m just going to land this if there are no objections by tonight.
|
|
@gvanrossum: Please replace |
This is a modification of the
LOAD_ATTRinline cache that landed a few months ago (PR 22803).The design overloads the
hintfield: if it is negative, it indicates a slot. Slots are created using__slots__, but some builtin types that usePyMemberDescrObjectdescriptors (with the type set toT_OBJECT_EX) will also benefit.I've tried to keep the code style similar to the existing
LOAD_ATTRcache (happy path first) but I had to make some compromises, e.g. the cache initialization code can no longer skip everything iftp_dictoffsetis zero (since it will be zero for classes with only slots).https://bugs.python.org/issue42927