Reduce memory usage by interning Tasks and RuleGraph entries. #14683
Conversation
This file contains 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
[ci skip-build-wheels]
benjyw
reviewed
Mar 3, 2022
benjyw
approved these changes
Mar 3, 2022
Will these leaks ever cause memory problems? |
No, I believe that this is a very safe case. We're interning But! That is why it wouldn't be a good idea to intern the entire |
This would be good to memorialize in comments. |
stuhood
added a commit
to stuhood/pants
that referenced
this pull request
Mar 3, 2022
…uild#14683) The `--stats-memory-summary` added in pantsbuild#14638/pantsbuild#14652 was [reporting surprisingly large sizes](pantsbuild#12662 (comment)) for native `NodeKey` structs -- even when excluding the actual Python values that they held. Investigation showed that both the `Task` and `Entry` structs were contributing significantly to the size of the `Task` struct. The [`internment` crate](https://crates.io/crates/internment) used here (and in pantsbuild#14654) is an alternative to giving these values integer IDs. They become pointers to a unique, shared (technically: leaked) copy of the value. They are consequently 1) much smaller, 2) much faster to compare. The `top`-reported memory usage of `./pants dependencies --transitive ::`: * `313M` before (summary [before.txt](https://github.com/pantsbuild/pants/files/8175461/before.txt)) * `220M` after (summary [after.txt](https://github.com/pantsbuild/pants/files/8175462/after.txt)) [ci skip-build-wheels]
stuhood
added a commit
that referenced
this pull request
Mar 3, 2022
…pick of #14683) (#14689) The `--stats-memory-summary` added in #14638/#14652 was [reporting surprisingly large sizes](#12662 (comment)) for native `NodeKey` structs -- even when excluding the actual Python values that they held. Investigation showed that both the `Task` and `Entry` structs were contributing significantly to the size of the `Task` struct. The [`internment` crate](https://crates.io/crates/internment) used here (and in #14654) is an alternative to giving these values integer IDs. They become pointers to a unique, shared (technically: leaked) copy of the value. They are consequently 1) much smaller, 2) much faster to compare. The `top`-reported memory usage of `./pants dependencies --transitive ::`: * `313M` before (summary [before.txt](https://github.com/pantsbuild/pants/files/8175461/before.txt)) * `220M` after (summary [after.txt](https://github.com/pantsbuild/pants/files/8175462/after.txt)) [ci skip-build-wheels]
stuhood
added a commit
that referenced
this pull request
Mar 20, 2022
…ased on the level (#14854) This change cleans up a few TODOs around workunit storage. Workunit names are almost entirely `&'static str`, with the notable exception of `NodeKey::Task` names (which are the Python function names of `@rules`). But #14683 began interning `Task` structs, which made `Task` names static as well. This change moves to storing workunit names as `&'static str`. Additionally, because the `Level` is a field of the `WorkunitMetadata`, the `WorkunitMetadata` was forced to be constructed, even for workunits whose level meant that they would not be rendered. Since the `WorkunitMetadata` contains heavy fields like the string description and user-metadata, this change moves to constructing the `WorkunitMetadata` via lazily evaluated `key=value` arguments to the `in_workunit!` macro (which also has the advantage of being more concise). Finally: an unused argument to `in_workunit!` is removed. Speeds up common runs by ~2%. [ci skip-build-wheels]
stuhood
added a commit
to stuhood/pants
that referenced
this pull request
Mar 29, 2022
…ased on the level (pantsbuild#14854) This change cleans up a few TODOs around workunit storage. Workunit names are almost entirely `&'static str`, with the notable exception of `NodeKey::Task` names (which are the Python function names of `@rules`). But pantsbuild#14683 began interning `Task` structs, which made `Task` names static as well. This change moves to storing workunit names as `&'static str`. Additionally, because the `Level` is a field of the `WorkunitMetadata`, the `WorkunitMetadata` was forced to be constructed, even for workunits whose level meant that they would not be rendered. Since the `WorkunitMetadata` contains heavy fields like the string description and user-metadata, this change moves to constructing the `WorkunitMetadata` via lazily evaluated `key=value` arguments to the `in_workunit!` macro (which also has the advantage of being more concise). Finally: an unused argument to `in_workunit!` is removed. Speeds up common runs by ~2%. [ci skip-build-wheels]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The
--stats-memory-summaryadded in #14638/#14652 was reporting surprisingly large sizes for nativeNodeKeystructs -- even when excluding the actual Python values that they held. Investigation showed that both theTaskandEntrystructs were contributing significantly to the size of theTaskstruct.The
internmentcrate used here (and in #14654) is an alternative to giving these values integer IDs. They become pointers to a unique, shared (technically: leaked) copy of the value. They are consequently 1) much smaller, 2) much faster to compare.The
top-reported memory usage of./pants dependencies --transitive :::313Mbefore (summary before.txt)220Mafter (summary after.txt)[ci skip-build-wheels]