Better handling in deduperreload for patching functions with freevars#14960
Conversation
smacke
force-pushed
the
fix-deduperreload-freevar-segfault
branch
from
7d5f1dd to
f6fd3f5
Compare
krassowski
left a comment
krassowski
left a comment
There was a problem hiding this comment.
Verified that this fixes the segfault for me:
smacke
force-pushed
the
fix-deduperreload-freevar-segfault
branch
from
60060d1 to
f6fd3f5
Compare
krassowski
left a comment
krassowski
left a comment
There was a problem hiding this comment.
This looks good to me, thank you @smacke. I have one question before merging regarding the pickle vs deepcopy usage.
| ) | ||
| func_asts = [ast.parse(func_code)] | ||
| if len(cast(ast.FunctionDef, func_asts[0].body[0]).decorator_list) > 0: | ||
| without_decorator_list = pickle.loads(pickle.dumps(func_asts[0])) |
There was a problem hiding this comment.
Is it preferred over copy.deepcopy? It feels like deepcopy would be faster?
There was a problem hiding this comment.
Hey @krassowski thanks for the review and sorry I missed this! It turns out that when it is applicable, composing pickle.loads and pickle.dumps should always be faster than copy.deepcopy, because the former uses a fast C implementation while the latter uses a slower Python implementation for recursively traversing objects. The drawback of pickle / unpickle is that it only works for pickle-able objects, but fortunately Python ASTs happen to be one such type of object.
This is coming from my memory so let me know if you'd prefer to see it for yourself via some benchmarks ;)
krassowski
changed the title
2 participants
This PR should fix #14958. When patching a decorated function, rather than giving it the union of the old version's and new version's freevars, we give it the old version's for anything callable, and the new version's for everything else. This ensure we use the old
__class__attribute and avoid errors such as "TypeError: super(type, obj): obj must be an instance or subtype of type". Using old versions of other callables is also better since these references are the ones we maintain in deduperreload, rather than the new references. It does, however, mean that we need to reload changed decorated functions twice: once without decorators (to ensure the old version gets patched), and once with. This PR handles that as well.Test plan: all the other unit tests pass + additionally added a new one which exercises this case.