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. |
|
If you want to schedule another build, you need to add the " |
|
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. |
|
If you want to schedule another build, you need to add the " |
|
If you want to schedule another build, you need to add the " |
| // 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. |
gvanrossum
Jan 28, 2021
Author
Member
Same way I missed it writing the code. :-(
This code is hairy.
| 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). |
gvanrossum
Jan 29, 2021
Author
Member
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
merged commit 5c5a938
into
python:master
|
@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