Fix reference cycle in Visitor's method cache - #371
Open
afonsojanu wants to merge 1 commit into
Open
Conversation
Visitor.visit() caches whichever visit_* method it resolves for a given node type on self._method_cache. Since the lookup goes through self, what gets cached is a bound method, and a bound method holds a reference back to the instance it belongs to. That means every TreeInterpreter ends up referencing itself through its own cache (self -> _method_cache -> bound method -> self), so it can only be freed by a cyclic GC pass instead of the usual refcounting. A TreeInterpreter gets built fresh on every ParsedResult.search() call, so under a busy workload this turns into a steady stream of avoidable GC pressure. Fetching the method off type(self) instead gives back a plain function with no bound reference to any instance, so the cache no longer creates a cycle. Calling it as method(self, node, ...) keeps dispatch working exactly the same as before, subclass overrides included. Added a regression test that spies on TreeInterpreter.__init__ to grab a weakref, runs a search, and checks the interpreter is already gone with gc disabled and no explicit collect - which only holds if nothing is keeping it alive through a cycle.
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
2 participants
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.
Follows up on #291. Visitor.visit() resolves the right visit_* method for a node type and stashes it in self._method_cache so the next node of the same type skips the getattr lookup. The lookup goes through self, though, which means what actually gets cached is a bound method, and a bound method carries a reference back to the instance it's bound to. So every TreeInterpreter ends up holding a reference to itself through its own cache, and since TreeInterpreter.search() builds a fresh interpreter on every call, that's a steady source of objects that can only be reclaimed by a cyclic GC pass instead of going away the moment their refcount hits zero. jamesls confirmed this with tracemalloc on the original issue and pointed straight at _method_cache as the main source of the extra allocations.
The fix looks the method up on type(self) instead, which gives back a plain function rather than a bound one, and calls it as method(self, node, ...). Dispatch behaves exactly the same, including for subclasses that override visit_* or default_visit, but nothing in the cache points back at an instance any more.
Added a test that spies on TreeInterpreter.init to grab a weakref, runs one search, and checks the interpreter is already gone with gc disabled and no explicit collect() call. That only holds if nothing is keeping it alive through a cycle, so it fails on the current code and passes with the fix.
Full test suite passes locally (993 tests, 1 pre-existing skip, both unaffected by this change).