Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Cached local process execution #7171

Conversation

cosmicexplorer
Copy link
Contributor

@cosmicexplorer cosmicexplorer commented Jan 28, 2019

WIP because this is truly massive and also addresses the tower protos migration a bit by converting Action and Command in remote.rs into the new protos (that usage was also moved to cached_execution.rs). That part wasn't intentional, but it happens to make all of the below a lot simpler and avoid converting between different proto types completely. I can clean this up if this is an appropriate scope.

Problem

Resolves #6898.

(explain the context of the problem and why you're making this change. include
references to all relevant github issues.
)

Solution

  • add an lmdb::Database to fs::local::ByteStore as a k/v store for bazel_protos::build::bazel::remote::execution::v2::Action{,Result} (from the new tower protos)
  • canonicalize encoding/decoding of lots of protos through public methods on fs::Store
  • create a new file cached_execution.rs which contains CacheableExecuteProcessRe{quest,sult} analogs to ExecuteProcessRequest and FallibleExecuteProcessResult, but which either contain caching information or don't contain information relevant to a specific process invocation, so that they can deterministically be serialized into Action{,Result} into the ByteStore
  • move a lot of proto encoding/decoding logic from remote.rs into cached_execution.rs
  • create CachingCommandRunner which impls process_execution::CommandRunner and serializes process invocations into Actions, and when complete, stores the result into an ActionResult in the process execution k/v db added to fs::local::ByteStore earlier.
  • switch usage of bazel_protos::remote_execution::{Action,Command} in remote.rs to the new tower protos

(describe the modifications you've made.)

Result

./pants -ldebug cloc :: is instantaneous with pantsd on, for one. This alone is a fantastic reason for tasks to consider switching to v2 rules, or at the very least a v2 process invocation (which is easier to keep cacheable when the process request is created using v2 rules). The only remaining bazel_protos::remote_execution::* protos I saw were Directory and File in fs::Store -- this is likely not hard to finish, as that remaining usage was still 100% orthogonal to this PR.

(describe how your changes affect the end-user behavior of the system. this section is
optional, and should generally be summarized in the title of the pull request.
)

@cosmicexplorer
Copy link
Contributor Author

Notes from @illicitonion on slack on how to get this mergeable:

Cool - FWIW one of the big advantages of separating things out is making it easier to review, so generally trying to do so before pushing things is handy… And if you can have your published commit history be useful standalone changes, that helps too…

Things I would separate out into separate PRs:
• Move encoding/decoding to Store
• Switch remote.rs to use new protos

Things I would separate out into separate commits:
• Add new lmdb to Store
• Create caching thing
• Wire up flags and context.rs so that the caching thing is actually used

I think ideally each of those bullet points would correspond to exactly one commit, and two of them would happen to also be other PRs…

@cosmicexplorer
Copy link
Contributor Author

I believe @hrfuller completed this work!

native-image for remote execution automation moved this from To do to Done Oct 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Local caching for ExecuteProcess
1 participant