Free typechecker state and ASTs when they are no longer needed #7280
Conversation
|
This cuts mypy's memory usage when doing a full check of S in half. |
There was a problem hiding this comment.
The overall idea seems sound to me, though I did have a few questions around how we're freeing the AST.
That said, I also no longer really have context on how anything related to incremental or fine-grained mode works, so it's unclear whether or not my opinion really means much here. Up to you if you want to merge now or if you want to wait for Ivan's review.
| self.update_fine_grained_deps(self.manager.fg_deps) | ||
| self.free_state() | ||
| if not manager.options.fine_grained_incremental and not manager.options.preserve_asts: | ||
| free_tree(self.tree) |
There was a problem hiding this comment.
Is there a reason why we can't do self.tree = None? Or maybe doing self.tree.defs.clear() or something?
There was a problem hiding this comment.
The reason we need to go deeper than just clearing defs is that the symbol table contains references to functions and classes and the like, and we need to clear out their bodies.
It would be possible to drive this from the symbol table, and I considered that, but it is more complex for not much gain. (Probably a bit faster, but probably doesn't matter)
(We actually probably should also do self.tree.defs.clear(), which would free a little bit more memory, though probably not a substantial amount.)
|
|
||
| class TreeFreer(TraverserVisitor): | ||
| def visit_block(self, block: Block) -> None: | ||
| super().visit_block(block) |
There was a problem hiding this comment.
Would clearing just the top-level blocks be sufficient here, instead of recursively clearing?
There was a problem hiding this comment.
We need to go recursively or we'll miss nested structures that appear in the symbol table.
|
Also, it could maybe be worth adding one or two tests that make sure reports aren't disrupted by this? I don't really remember when we accumulate stats for those/whether we end up needing full access to the ASTs after they're freed. (Not sure if the test failures are related to this potential failure mode -- they mostly just reminded me that reports are a thing.) |
Instead of hanging around to all of them for the entire run of the process, free ASTs and typechecker state (especially the type map) as soon as a module is finished being processing. In order to have this work when generating fine-grained dependencies, we need to produce fine-grained dependencies much earlier, so BuildManager now has an `fg_deps` field. In the daemon, only free typechecker state, since we want to keep ASTs around to increase recheck speed. (A future change might use an LRU cache to keep only some around.)
msullivan
force-pushed
the
free-more-memory
branch
from
9a57433 to
9a1f7ec
Compare
|
I'm going to go ahead and merge this so that I can catch it in an internal release, but I'm happy to make any additional revisions if Ivan wants next week |
|
This is a nice win! |
|
|
||
| This needs to be done recursively, since symbol tables contain | ||
| references to definitions, so those won't be freed but we want their | ||
| contents to be. |
There was a problem hiding this comment.
We might fix this in long term, see #5159, but definitely not right now. I don't have any additional comments here, thanks for improving this!
Instead of hanging around to all of them for the entire run of the
process, free ASTs and typechecker state (especially the type map) as
soon as a module is finished being processing.
In order to have this work when generating fine-grained dependencies,
we need to produce fine-grained dependencies much earlier, so
BuildManager now has an
fg_depsfield.In the daemon, only free typechecker state, since we want to keep ASTs
around to increase recheck speed. (A future change might use an LRU
cache to keep only some around.)