[internal] Implement `Snapshot` in terms of a new `DigestTrie` type #14654

stuhood · 2022-03-01T01:01:49Z

As described in #13112: we currently persist all remexec::Directory structures to disk, although many of them are intermediate results which never actually need to be persisted across restarts (the only position which actually needs to be persisted is a cache output). That extra IO (and complexity) has performance costs: roughly 20% of some profiles.

In order to skip persisting remexec::Directorys to disk, we need an in-memory structure to replace them (which we will persist or load from a Store when necessary). To that end, this change introduces a DigestTrie type, which is a trie representing a Digested filesystem tree. A DigestTrie is sufficient to replace the list of PathStats previously held by a Snapshot. But because it uses interned names for path members, and is structure shared using Arcs, it does so using a lot less memory.

This is the first of three or four PRs which will replace all methods which recursively load/store remexec::Directory with operations on DigestTries instead (e.g. MergeDigests, DigestSubset, etc). As methods are ported, their boundaries will be adjusted to skip calling store.record_digest_tree (which persists the DigestTree to disk using our existing format), until only the local cache persists directory structures.

stuhood · 2022-03-01T22:55:31Z

src/rust/engine/fs/src/directory.rs

+  /// Creates a DirectoryDigest which asserts that the given Digest represents a Directory structure
+  /// which is persisted in a Store.
+  pub fn new(digest: Digest) -> Self {
+    Self { digest, tree: None }
+  }


This method could almost have been called DirectoryDigest::todo_port_to_consuming_digest_trees, as all usages other than usage at a remote execution boundary represent positions where we should be holding a DirectoryDigest already.

I like that name, if it isn't too much of a pain to update w/ merge conflicts.

I went ahead and did (roughly) this, because it will really help with tracking down the various cases where we are converting.

stuhood · 2022-03-01T22:57:21Z

src/rust/engine/fs/src/directory.rs

+  fn from_sorted_paths(
+    prefix: PathBuf,
+    paths: Vec<TypedPath>,
+    file_digests: &HashMap<PathBuf, Digest>,
+  ) -> Self {


This method was ported from Snapshot::ingest_sorted_path_stats. Most of #13112 involves porting methods like this from operating against the Store (and lazy loading/storing remexec::Directorys) to synchronous in-memory operations.

Eric-Arellano · 2022-03-01T23:35:12Z

I know this is one of several PRs, but thoughts on a benchmark?

stuhood · 2022-03-01T23:37:43Z

I know this is one of several PRs, but thoughts on a benchmark?

I'm fairly certain that this will be slower, since it is both creating the tree in memory and then persisting it to disk. It basically only adds work. The next PR will begin to remove work by allowing the tree to be used in e.g. MergeDigests without being loaded: will include benchmarks there.

We also don't have to land this until more of the followup work is posted.

Eric-Arellano

Looks good. I do recommend considering the variable/function renames here, but fine to punt on unit tests in a followup if necessary.

src/rust/engine/fs/src/tree.rs

src/rust/engine/fs/src/directory.rs

Eric-Arellano · 2022-03-02T00:05:32Z

src/rust/engine/fs/src/directory.rs

+  /// Creates a DirectoryDigest which asserts that the given Digest represents a Directory structure
+  /// which is persisted in a Store.
+  pub fn new(digest: Digest) -> Self {
+    Self { digest, tree: None }
+  }


I like that name, if it isn't too much of a pain to update w/ merge conflicts.

src/rust/engine/fs/src/directory.rs

Eric-Arellano · 2022-03-02T00:11:02Z

src/rust/engine/fs/src/directory.rs

+  ///
+  /// If this DirectoryDigest has been persisted to disk (i.e., does not have a DigestTrie) then
+  /// this will only include the root.
+  pub fn digests(&self) -> Vec<Digest> {


Maybe test this? The stack code looks good but is non-trivial.

src/rust/engine/fs/src/directory.rs

Eric-Arellano

~~Unit tests would still be really good to add in a followup because there are lots of small functions w/ somewhat non-trivial functionality. But fine for a followup~~

src/rust/engine/fs/src/directory.rs

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]

…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]

…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]

[ci skip-build-wheels]

…ore a `DirectoryDigest` in `PyDigest`. [ci skip-build-wheels]

[ci skip-build-wheels]

stuhood · 2022-03-04T20:53:56Z

I know this is one of several PRs, but thoughts on a benchmark?

I'm fairly certain that this will be slower, since it is both creating the tree in memory and then persisting it to disk. It basically only adds work. The next PR will begin to remove work by allowing the tree to be used in e.g. MergeDigests without being loaded: will include benchmarks there.

We also don't have to land this until more of the followup work is posted.

I've posted a few more PRs from the stack, and it looks like although performance is not better (yet!) it at least hasn't regressed: #13112 (comment)

Landing.

stuhood force-pushed the stuhood/digest-tree branch 5 times, most recently from ed91450 to 4ddf601 Compare March 1, 2022 21:23

stuhood changed the title ~~Implement Snapshot in terms of a new DigestTree type~~ Implement Snapshot in terms of a new DigestTrie type Mar 1, 2022

stuhood requested review from Eric-Arellano, tdyas and jsirois March 1, 2022 22:47

stuhood marked this pull request as ready for review March 1, 2022 22:47

stuhood mentioned this pull request Mar 1, 2022

Move to an in-memory, native Directory structure (aka: lazily persist Directories) #13112

Closed

stuhood commented Mar 1, 2022

View reviewed changes

Eric-Arellano approved these changes Mar 2, 2022

View reviewed changes

tdyas approved these changes Mar 2, 2022

View reviewed changes

src/rust/engine/fs/src/directory.rs Outdated Show resolved Hide resolved

src/rust/engine/fs/src/directory.rs Outdated Show resolved Hide resolved

stuhood force-pushed the stuhood/digest-tree branch from a3bb4c4 to f713e82 Compare March 2, 2022 18:32

Eric-Arellano approved these changes Mar 2, 2022

View reviewed changes

stuhood force-pushed the stuhood/digest-tree branch from f713e82 to 9ca6a49 Compare March 2, 2022 18:36

tdyas reviewed Mar 2, 2022

View reviewed changes

src/rust/engine/fs/src/directory.rs Outdated Show resolved Hide resolved

src/rust/engine/fs/src/directory.rs Show resolved Hide resolved

stuhood force-pushed the stuhood/digest-tree branch from 9ca6a49 to 8e6137b Compare March 2, 2022 19:01

stuhood mentioned this pull request Mar 3, 2022

Reduce memory usage by interning Tasks and RuleGraph entries. #14683

Merged

stuhood mentioned this pull request Mar 3, 2022

Reduce memory usage by interning Tasks and RuleGraph entries. (cherrypick of #14683) #14689

Merged

stuhood changed the title ~~Implement Snapshot in terms of a new DigestTrie type~~ [internal] Implement Snapshot in terms of a new DigestTrie type Mar 4, 2022

stuhood added 3 commits March 3, 2022 16:46

Add (unused) DigestTrie struct.

96ad80f

[ci skip-build-wheels]

Implement the Snapshot node in terms of a DirectoryDigest, and st…

44b3ffc

…ore a `DirectoryDigest` in `PyDigest`. [ci skip-build-wheels]

store::Snapshot holds a DigestTree rather than PathStats.

d190d3a

[ci skip-build-wheels]

Review feedback.

1e033f2

[ci skip-build-wheels]

stuhood force-pushed the stuhood/digest-tree branch from 8e6137b to 1e033f2 Compare March 4, 2022 00:52

jsirois approved these changes Mar 4, 2022

View reviewed changes

stuhood merged commit b599eb5 into pantsbuild:main Mar 4, 2022

stuhood deleted the stuhood/digest-tree branch March 4, 2022 20:54

Eric-Arellano mentioned this pull request Mar 4, 2022

Prepare 2.11.0.dev3 #14708

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[internal] Implement `Snapshot` in terms of a new `DigestTrie` type #14654

[internal] Implement `Snapshot` in terms of a new `DigestTrie` type #14654

stuhood commented Mar 1, 2022 •

edited

Loading

stuhood Mar 1, 2022

Eric-Arellano Mar 2, 2022

stuhood Mar 2, 2022

stuhood Mar 1, 2022

Eric-Arellano commented Mar 1, 2022

stuhood commented Mar 1, 2022

Eric-Arellano left a comment

Eric-Arellano Mar 2, 2022

Eric-Arellano Mar 2, 2022

Eric-Arellano left a comment •

edited

Loading

stuhood commented Mar 4, 2022

[internal] Implement Snapshot in terms of a new DigestTrie type #14654

[internal] Implement Snapshot in terms of a new DigestTrie type #14654

Conversation

stuhood commented Mar 1, 2022 • edited Loading

stuhood Mar 1, 2022

Choose a reason for hiding this comment

Eric-Arellano Mar 2, 2022

Choose a reason for hiding this comment

stuhood Mar 2, 2022

Choose a reason for hiding this comment

stuhood Mar 1, 2022

Choose a reason for hiding this comment

Eric-Arellano commented Mar 1, 2022

stuhood commented Mar 1, 2022

Eric-Arellano left a comment

Choose a reason for hiding this comment

Eric-Arellano Mar 2, 2022

Choose a reason for hiding this comment

Eric-Arellano Mar 2, 2022

Choose a reason for hiding this comment

Eric-Arellano left a comment • edited Loading

Choose a reason for hiding this comment

stuhood commented Mar 4, 2022

[internal] Implement `Snapshot` in terms of a new `DigestTrie` type #14654

[internal] Implement `Snapshot` in terms of a new `DigestTrie` type #14654

stuhood commented Mar 1, 2022 •

edited

Loading

Eric-Arellano left a comment •

edited

Loading