Repository navigation
gh-124697: avoid unnecessary deduplication in frameobject API functions - #160256
iritkatriel wants to merge 2 commits into
Conversation
| if (PyErr_Occurred()) { | ||
| return -1; | ||
| } | ||
| if (PyDict_SetItem(seen, name, has_value ? Py_True : Py_False) < 0) { |
There was a problem hiding this comment.
While the cache is UNKNOWN, this inserts an entry even for a local that hasn't been bound yet. A debugger enumerating locals on function entry can hit this with most slots still empty. I measured the first len(proxy) on a generator suspended at its first yield, with one bound argument and 256 unbound locals. The median went from 149 ns on the parent to 6,239 ns on this PR, and peak traced allocation during the call went from 216 to 9,776 bytes. I used matched GIL-enabled macOS arm64 release builds, seven runs per revision, and a fresh code object for each first read. Frame and proxy creation were outside the timer.
Could we track just the bound names on a sparse walk, mark NEEDED if two of them match, and otherwise leave the cache UNKNOWN until a walk can establish uniqueness?
| _PYCODE_LOCALSPLUS_DEDUP_UNIQUE) { | ||
| return 0; | ||
| } | ||
| *seen = PyDict_New(); |
There was a problem hiding this comment.
Once the cache says NEEDED, we skip unbound slots, so every value we put in this dict is True. Could we keep the old set of bound names for that case and only write NEEDED when we first discover a duplicate?
I timed repeated reads inside [probe(sys._getframe().f_locals) for x in x], with 14 bound arguments plus the comprehension target and the enclosing x. Both x slots were live. These are median times per call over seven runs per revision on GIL-enabled macOS arm64 release builds.
| Call | PR parent | This PR |
|---|---|---|
len(proxy) |
191 ns | 345 ns |
proxy.keys() |
239 ns | 390 ns |
proxy.values() |
243 ns | 391 ns |
proxy.items() |
346 ns | 507 ns |
Changing only the target to y gives 16 unique names and shows the intended speedup for len(proxy), from 198 ns to 25 ns.
| code = f""" | ||
| def outer(x): | ||
| def inner(): | ||
| {probe} |
There was a problem hiding this comment.
Could we assert the result of this first probe too? I tried making framelocalsproxy_visit_name() return 0 whenever the name was already in seen. It still learned NEEDED, so all four subtests passed even though the first keys(), values(), and items() calls returned [] and the first len() returned 0.
The enclosing x is already bound, so I'd expect ['x'], [[7, 8]], [('x', [7, 8])], and 1, respectively. Small wording nit, too. inner.__code__.co_cellvars is empty in this example, so “slot” would describe the comprehension target more accurately in the test name and comment.
Cache on the code object whether deduplication is needed. Skip it if not.
This should resolve a perf regression introduced in #156819