Inlining of PathGlob expansion causes excess invalidation with the daemon

[stuhood]

Inlining of PathGlobsExpansion causes Snapshots to be more fragile than necessary. In particular, because VFS::expand recursively walks from the root down to the relevant directories without memoizing PathGlobsExpansions, any change in the root invalidates all Snapshots from a changed path up to the root of the repo (but not the directory listings and stats).

This significantly affects warm performance in the presence of the daemon.

We should experiment with memoizing (portions of?) PathGlobsExpansion in the Graph (similar to what we do for ReadLink/Scandir), and see how much it affects cold/warm performance.

NB: As a wildcard: it's possible that fixing #4558 would be a better approach than increasing memoization would be.

---

The relevant codepath centers around

pants/src/rust/engine/fs/src/lib.rs

Lines 570 to 576 in cdba694

	///
	/// Recursively expands PathGlobs into PathStats while applying excludes.
	///
	/// TODO: Eventually, it would be nice to be able to apply excludes as we go, to
	/// avoid walking into directories that aren't relevant.
	///
	fn expand(&self, path_globs: PathGlobs) -> BoxFuture<Vec<PathStat>, E> {

. When we call that method from the engine (here:

pants/src/rust/engine/src/nodes.rs

Lines 913 to 940 in 01bcda6

	// Recursively expand PathGlobs into PathStats while tracking their dependencies.
	context
	.expand(path_globs)
	.then(move |path_stats_res| match path_stats_res {
	Ok(path_stats) => {
	// Declare dependencies on the relevant Stats, and then create a Snapshot.
	let stats = future::join_all(
	path_stats
	.iter()
	.map(
	|path_stat| context.get(Stat(path_stat.path().to_owned())), // for recording only
	)
	.collect::<Vec<_>>(),
	);
	// And then create a Snapshot.
	stats
	.and_then(move |_| {
	context
	.core
	.snapshots
	.create(&context.core.vfs, path_stats)
	.map_err(move |e| throw(&format!("Snapshot failed: {}", e)))
	})
	.to_boxed()
	}
	Err(e) => err(throw(&format!("PathGlobs expansion failed: {:?}", e))),
	})
	.to_boxed()

), the VFS in question is

pants/src/rust/engine/src/nodes.rs

Line 73 in 01bcda6

impl VFS<Failure> for Context {

, which has the effect of memoizing/recording ReadLink and Scandir calls in the Graph (which later allows for filesystem invalidation).

The effect of memoizing ReadLink and Scandir (but none of the rest of the other steps of a PathGlobsExpansion) is that a Snapshot Node in the Graph is flat: the Snapshot depends directly on the syscalls, and to recompute it the entire VFS::expand call reruns.

---

When we fix this, it would be good to get a simple benchmark in place on the rust side. But it's also important to directly measure the performance of:

• cold ./pants list ::
• warm ./pants list :: after touching one file at the root of the repo

[stuhood]

This is primarily annoying when changing files in the root of the repo, which is relatively rare. And since it doesn't effect correctness, going to defer to 1.4.0.

[stuhood]

@jsirois : One thing to be cautious of: @illicitonion will be changing this code a bit in order to integrate it with #4585. The memoization that is done in this codepath will continue to be necessary to provide good incremental performance in the context of the engine though, so that part will not change.

@illicitonion : Worth being aware of this ticket, as it explains a bit of why the fs module does what it does.

[stuhood]

This will be resolved by #5106.

[kwlzn]

assuming fixed by #5106 (not sure why github didn't auto-close based on the annotation) - thanks @illicitonion !

[kwlzn]

err, guess that was because #5106 was 'closed' vs 'merged' and superceded by #5496.

[stuhood]

So, it looks like this is still an issue, in that touching a file in the root of the repository will invalidate the listing there, which will invalidate all Snapshots, because they depend on the filesystem operations. Basically everything else in the graph depends on a Snapshot, so the result is that although only one new IO happens, we end up re-constructing the Snapshots (which requires some round trips to the database) and all rule logic re-runs.

Fixing #4558 seems like the least contorted way to resolve this, so will re-open this until we resolve #4558. cc @jsirois