Repository navigation
Store both function and code object in the function version cache #117045
Description
Activity
This can use some (internal) API design. I have a draft PR (gh-109998) where I attempt to preserve most of the original API. This leads to some oddities.
_PyFunction_SetVersion(func, version)has the same interface as before, but does a bit more. It evictsfuncfrom the version cache if its previousfunc_versionwas nonzero (and the slot actually refers tofunc), leaving the correspondingcodethere (this is the crucial new feature); it storesfuncandfunc->func_codein the cache (overwriting both) if the new version is nonzero._PyFunction_ClearCodeByVersion(version)(New:) evicts the code object from the cache, if the slot actually refers to a code object with that version. Design question: should this function take the code object or the version as argument? It works either way (since code objects versions are unique) but perhaps it should take the code object so we can check that the slot refers to the given code object. EDIT: And should it also clear the correspondingfunccache entry?_PyFunction_LookupByVersion(version, p_code)(Changed:) has grown an output parameter through which the code object is delivered. Design question: Maybe this should have two output parameters (p_func,p_code) and return a status indicating success (where success is defined as "at least the code object returned is non-NULL")?_PyFunction_GetVersionForCurrentState(func)has the same interface as before, but just returnsfunc->func_version-- it no longer attempts to assign a function object a new version when its version is zero. Design question: Maybe this should get a more logical name?
Final design question: Is it better to just set
func_version(and insert the function and code objects into the cache) inPyFunction_New[WithQualName](), so we don't have to bother setting the version in theMAKE_FUNCTIONopcode? This makes things a tad cleaner (notably for the JIT) but slightly complicates function creation (to insert it into the cache). A worst-case scenario would be where 3rd party C code creates a large number of code and function objects that it doesn't intend to be called by the bytecode interpreter (maybe they are passed around and called usingPyObject_Call()), evicting the entire cache. For this reason I think I will keep this inMAKE_FUNCTION.One thought I had while reading: is it going to bite us down the road that we can only cache one function per code object? This could potentially be an issue if somebody is creating inner functions or generators with overlapping lifetimes. If not, should we really be treating them all interchangably, since we could end up looking up the "wrong" instance of a given inner function?
(The answer may be "it's fine", but it seemed like something worth considering at this point in the design process.)
For example, I think I found a way to create two functions with the same version, but different numbers of defaults:
>>> def outer(): ... def inner(x=None): ... pass ... return inner ... >>> a = outer() >>> outer.__code__ = outer.__code__.replace(co_consts=(*outer.__code__.co_consts[:2], ())) >>> b = outer() >>> a.__defaults__ (None,) >>> b.__defaults__ ()
Oof:
>>> def outer(): ... def inner(x=None): ... pass ... return inner ... >>> a = outer() >>> outer.__code__ = outer.__code__.replace(co_consts=(*outer.__code__.co_consts[:-1], (None, None))) >>> b = outer() >>> a.__defaults__ (None,) >>> b.__defaults__ (None, None) >>> def loop(f): ... for _ in range(1000): ... f() ... >>> loop(a) >>> loop(b) python: Python/generated_cases.c.h:1824: _PyEval_EvalFrameDefault: Assertion `defcount <= code->co_argcount' failed. Aborted
We might consider being more careful about using code and function versions interchangeably. :(
Ouch. That crashes on main too.
Maybe that assert should just be deleted? There's a corresponding DEOPT_IF() two lines below.EDIT: It's not the same. But it could be replaced by DEOPT_IF().
I think you just found a bug in the specializer. :)
I think it's just an incorrect assert? The rest of the code works with wonky numbers of defaults.
Reacted by Guido van RossumAgreed.
I think there might be a sneakier way.
The idea is that we can use
compile(src, file, "exec")to create a code object that we canexec()in different namespaces (i.e., globals dicts). They will share a code object which incorporates an executor which references a global constant with_LOAD_CONST_INLINE. We can re-execute it in another namespace where the global has a different value, but the executor is the same and the dict watcher is still watching the original namespace. Behold: this should print1999 2999Code:
src = """ def f(): global x def inner(i): a = 1 for _ in range(100): a = x + i return a return inner func = f() """ co = compile(src, __file__, "exec") ns1 = {"x": 1000} eval(co, ns1) func1 = ns1["func"] for i in range(1000): x = func1(i) print(x) ns2 = {"x": 2000} eval(co, ns2) func2 = ns2["func"] for i in range(1000): x = func2(i) print(x)
But with this PR it prints
1999 1999Again, this is also broken on main. :-( I've filed gh-117051; I don't think it's an easy one though (maybe for Mark :-).
- added 2 commits that reference this issue
on Apr 2, 2024
(Copied from faster-cpython/ideas#665.)
The idea here is to avoid function version cache misses for generator expressions. (See faster-cpython/ideas#664 (comment).)
We have a complicated mechanism to reset the function version whenever
__code__,__defaults__and a few other critical attributes are mutated. (BTW: nothing is affected by changes to__annotations__, and yet that is also considered critical; I will fix this.)Why not instead just reset the function version to zero and stick to that? We then guarantee that the function version is either zero or matches the code object version.
Nothing changes for specialization except that
_PyFunction_GetVersionForCurrentState()returns 0 for mutated functions. This is unlikely to affect any benchmark or other perf-critical real-world code.The function version cache will double in size, and store both the function and the code object. When a function is deallocated or its version is reset to zero, it evicts itself from the cache, but keeps the code object. Code objects remove themselves from the cache when deallocated (and probably also evict the function object).
For Tier 2, when generating
_PUSH_FRAMEor_POP_FRAME, we can handle the case where the function version maps to a pair(NULL, some_code_object)-- we storeNULLin the operand, but we usesome_code_objectto trace through. Globals removal will no longer work (boo hoo), but at least we still have a trace. Presumably at least some generator expressions don't use globals (they can still use builtins, which can be reached without the function object).Linked PRs