Add caching CommandRunner wrapper #7911

illicitonion · 2019-06-20T14:27:34Z

This allows any CommandRunner to be wrapped and transparently cache
all process executions it performs.

illicitonion · 2019-06-20T14:30:15Z

Bottom 3 commits are dependent PRs (#7908, #7909, #7910), top commit has the meat.

Top commit has two things mixed into it:

Move some code in remote.rs to live outside of the remote::CommandRunner struct, and change error types to by String instead of ExecutionError::Fatal(String) (and otherwise not make any changes), so it can be re-used.
Add cache.rs which re-uses that code.

stuhood

Thanks! One blocker I think, but otherwise looks good.

stuhood · 2019-06-21T18:55:36Z

src/rust/engine/process_execution/src/cache.rs

+
+impl CommandRunner {
+  fn digest(&self, req: &ExecuteProcessRequest) -> Result<Digest, String> {
+    // TODO: Parameterise options


Yep. Passing ~all of these looks like a blocker for landing.

stuhood · 2019-06-21T19:02:17Z

src/rust/engine/process_execution/src/cache.rs

+      directory
+    });
+    let process_execution_store = self.process_execution_store.clone();
+    self


This should probably use a different default_lease_until_secs_since_epoch value than everything else in the store. Best as a followup though.

Added a TODO. Also added a TODO to ever GC this store.

Hm... Why wouldn't this be affected by the existing GC...?

Currently garbage collection is exposed by Store (which is a layer on top of ShardedLmdb), and this uses a separate ShardedLmdb. It's easy to add GCing, but this currently isn't done.

stuhood · 2019-06-21T19:03:56Z

src/rust/engine/process_execution/src/cache.rs

+      .join(
+        self
+          .file_store
+          .store_file_bytes(result.stderr.clone(), true),


If we fail to get any of these, we should probably just treat it as a cache miss rather than a failure (possibly with debug or trace logging), because I can definitely imagine some of the dependency data expiring out here a little bit out of order with the actual cache entry.

illicitonion · 2019-07-10T16:31:17Z

Now depends on #8035 then should be good to go :)

stuhood

Thanks!

stuhood · 2019-07-10T21:00:35Z

src/rust/engine/process_execution/src/cache.rs

+      directory
+    });
+    let process_execution_store = self.process_execution_store.clone();
+    self


Hm... Why wouldn't this be affected by the existing GC...?

stuhood · 2019-07-10T21:04:51Z

src/rust/engine/process_execution/src/remote.rs

+          stdout_digest, error
+        )
+      })
+      .and_then(move |maybe_value| {


Should this method and branch return an Option instead so that this case can be handled differently by the caller for the cache vs other cases? Ditto the other methods.

Can do, but the only difference I can really imagine is logging verbosity; I would expect None to not-or-barely log, but Err to log saying there was a communication error. We can happily do this as a follow-up if we decide to change the caller's behaviour :)

I was thinking that it would avoid creating error strings for cache misses. But yea, minor.

This allows any `CommandRunner` to be wrapped and transparently cache all process executions it performs. Still TODO: * Wire up Core to actually use this (probably behind a flag).

illicitonion requested review from blorente, Eric-Arellano and cattibrie June 20, 2019 14:27

illicitonion requested a review from gshuflin June 21, 2019 18:49

stuhood reviewed Jun 21, 2019

View reviewed changes

illicitonion force-pushed the dwagnerhall/local-process-execution-cache/cache branch from 7c45fb2 to 99ab171 Compare June 21, 2019 23:46

illicitonion force-pushed the dwagnerhall/local-process-execution-cache/cache branch from 99ab171 to e42f5b2 Compare July 10, 2019 16:29

stuhood approved these changes Jul 10, 2019

View reviewed changes

Add caching CommandRunner wrapper

c3d2501

This allows any `CommandRunner` to be wrapped and transparently cache all process executions it performs. Still TODO: * Wire up Core to actually use this (probably behind a flag).

illicitonion force-pushed the dwagnerhall/local-process-execution-cache/cache branch from e42f5b2 to c3d2501 Compare July 11, 2019 11:13

illicitonion merged commit fd346c5 into pantsbuild:master Jul 11, 2019

illicitonion deleted the dwagnerhall/local-process-execution-cache/cache branch July 11, 2019 17:26

stuhood mentioned this pull request Jul 27, 2019

Local caching for ExecuteProcess #6898

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add caching CommandRunner wrapper #7911

Add caching CommandRunner wrapper #7911

illicitonion commented Jun 20, 2019 •

edited

Loading

illicitonion commented Jun 20, 2019

stuhood left a comment

stuhood Jun 21, 2019

illicitonion Jul 10, 2019

stuhood Jun 21, 2019

illicitonion Jul 10, 2019

stuhood Jul 10, 2019

illicitonion Jul 11, 2019

stuhood Jun 21, 2019

illicitonion Jul 10, 2019

illicitonion commented Jul 10, 2019

stuhood left a comment

stuhood Jul 10, 2019

stuhood Jul 10, 2019

illicitonion Jul 11, 2019

stuhood Jul 11, 2019

Add caching CommandRunner wrapper #7911

Add caching CommandRunner wrapper #7911

Conversation

illicitonion commented Jun 20, 2019 • edited Loading

illicitonion commented Jun 20, 2019

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

illicitonion commented Jul 10, 2019

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

illicitonion commented Jun 20, 2019 •

edited

Loading