bpo-46841: Use inline caching for attribute accesses - #31640
Merged
brandtbucher merged 12 commits intoMar 3, 2022
Merged
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
brandtbucher
commented
Mar 2, 2022
Comment thread
Python/ceval.c
Outdated
Comment on lines
+1462
to
+1463
| /* XXX: Remove this next line to make test_asyncio very angry! */ \ | ||
| DEOPT_IF(LOAD_##attr_or_method == LOAD_ATTR, LOAD_##attr_or_method); \ |
Copy link
Copy Markdown
Member
Author
There was a problem hiding this comment.
All tests pass if we deopt every LOAD_ATTR_MODULE right here. Removing this line activates some very strange failures!
Sorry, something went wrong.
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
Casting a _PyLoadMethodCache to a _PyAttrCache may not be valid.
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
|
(We're also going to want to hit this with the buildbots once it's working.) |
Sorry, something went wrong.
markshannon
requested changes
Mar 2, 2022
markshannon
left a comment
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
I like the change to use arrays in the structs. It's much clearer that my _m1 field.
But, be careful of casting one struct to another, and be aware that the C compiler treats arrays as simple pointers.
Sorry, something went wrong.
Comment thread
Include/internal/pycore_code.h
Comment thread
Include/internal/pycore_code.h
Outdated
Comment thread
Include/internal/pycore_code.h
Outdated
Comment thread
Lib/opcode.py
| def_op('BUILD_SET', 104) # Number of set items | ||
| def_op('BUILD_MAP', 105) # Number of dict entries | ||
| name_op('LOAD_ATTR', 106) # Index in name list | ||
| name_op('LOAD_ATTR', 106, 4) # Index in name list |
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
The existence of the LOAD_MODULE_ATTR_OR_METHOD macro implies that the LOAD_ATTR and LOAD_METHOD caches should have the same layout. Yet, this has size 4 and LOAD_METHOD has size 10.
This may be the source of your problem.
Are you using the two version numbers consistently?
Sorry, something went wrong.
Comment thread
Include/internal/pycore_code.h
| _Py_CODEUNIT dict_offset; | ||
| _Py_CODEUNIT keys_version[2]; | ||
| _Py_CODEUNIT descr[4]; | ||
| } _PyLoadMethodCache; |
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
Given that LOAD_METHOD_MODULE and LOAD_ATTR_MODULE share code, maybe change this to:
typedef struct {
_PyAttrCache attr;
_Py_CODEUNIT keys_version[2];
_Py_CODEUNIT descr[4];
} _PyLoadMethodCache;
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
There was a problem hiding this comment.
I'd rather leave it as-is. Otherwise, accessing the attr methods, including the counter, is a bit awkward.
In response to your other comments: I don't think we're doing anything illegal here, since we only use one consistent cache layout for each specialized form (meaning, _PyLoadMethodCache and _AttrCache members never alias each other). The exception is the counter member for all instructions, but C guarantees that it the first member of a struct will always be at offset 0. So no issues there.
Sorry, something went wrong.
Comment thread
Python/ceval.c
Outdated
Comment on lines
+1462
to
+1463
| /* XXX: Remove this next line to make test_asyncio very angry! */ \ | ||
| DEOPT_IF(LOAD_##attr_or_method == LOAD_ATTR, LOAD_##attr_or_method); \ |
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
Casting a _PyLoadMethodCache to a _PyAttrCache may not be valid.
Sorry, something went wrong.
Copy link
Copy Markdown
|
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again. |
Sorry, something went wrong.
26 tasks
Copy link
Copy Markdown
Member
Author
Okay, that was officially the wildest bug I've ever dealt with. The problematic bytecode sequence (buried deep in asyncio) was: LOAD_ATTR [CACHE] [CACHE] [CACHE] [CACHE] LOAD_FAST LOAD_ATTR Quickened: LOAD_ATTR_ADAPTIVE [CACHE] [CACHE] [CACHE] [CACHE] LOAD_FAST LOAD_ATTR_ADAPTIVE After specializating the first LOAD_ATTR_ADAPTIVE: LOAD_ATTR_MODULE [<counter>] [<version[0]>] [<version[1]>] [<index>] LOAD_FAST LOAD_ATTR_ADAPTIVE Finally, after specializing the second LOAD_ATTR_ADAPTIVE: LOAD_ATTR_MODULE [<counter>] [<version[0]>] [<version[1]>] [LOAD_FAST] LOAD_FAST__LOAD_ATTR_INSTANCE_VALUE LOAD_ATTR_INSTANCE_VALUE Did you catch that? The cached index was turned into a LOAD_FAST! The code that did that is here. Basically, the low byte of the cached index just happened to be LOAD_FAST__LOAD_FAST, so the LOAD_FAST__LOAD_ATTR_INSTANCE_VALUE specialization code changed it "back" into a LOAD_FAST. What it really did, though, was change the module attribute cached index from 169 to 124. This also just happened to be a valid index into the os module's __dict__, and on subsequent runs the code ended up calling os.mknod(self._fileno, False) instead of os.set_blocking(self._fileno, False). This silently "works", and causes the code to leak tons of resources and break later in more subtle ways. Phew. |
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
|
This whole incident highlights one limitation with the new inline caching approach: it's no longer safe to peek backwards at previous instructions, since you might actually be looking into a cache entry instead. I don't see how we can keep LOAD_FAST__LOAD_ATTR_INSTANCE_VALUE without making LOAD_FAST adaptive (and even then, it will interact poorly with a previous LOAD_FAST__LOAD_FAST), so it probably just makes the most sense to just remove it for now. I'll also go over the existing specialization code and see if we're doing this anywhere else. |
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
|
It also highlights the need for a quickened=True option in dis that we talked about before. This was a huge pain to debug by hand... _co_quickened helped a lot, though! |
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
|
Looping in @sweeneyde, since I might need to get rid of LOAD_FAST__LOAD_ATTR_INSTANCE_VALUE as part of this PR (see my comments above). |
Sorry, something went wrong.
Copy link
Copy Markdown
Member
I didn't see the macro benchmark needle move too much when it was added, so if it's architecturally necessary to, I have no problem with it being scrapped for now. |
Sorry, something went wrong.
Copy link
Copy Markdown
|
🤖 New build scheduled with the buildbot fleet by @brandtbucher for commit 452c78c 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
|
Buildbot failures look unrelated. x86 Gentoo Installed with X PR was just an SSL timeout, and AMD64 Arch Linux Usan PR is failing with the same errors in other PRs. |
Sorry, something went wrong.
markshannon
reviewed
Mar 3, 2022
Comment thread
Python/specialize.c
| return -1; | ||
| } | ||
| if (err) { | ||
| if (_Py_OPCODE(instr[0]) == LOAD_ATTR_INSTANCE_VALUE) { |
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
This is a common pair and it saves a POP/PUSH pair and an incref/decref pair, so I'd like to revisit this in the future.
Do we have an issue for that?
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
There was a problem hiding this comment.
Discussion: faster-cpython/ideas#291
Issue: https://bugs.python.org/issue46823
Sorry, something went wrong.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CC @markshannon. These three share a lot of code, so it makes sense to convert them together.
Marking as DO-NOT-MERGE since LOAD_ATTR_MODULE is somehow causing assertion failures and unclosed resources in test_asyncio:
DetailsAny ideas on why that might be are definitely appreciated... I've been staring at this for a while and am completely at a loss. The rest of the PR is ready for review, though.
https://bugs.python.org/issue46841