I've been playing with remote cache for the last few days and I have two thoughts: 1. Some processes should not be remote cached? like finding binaries on the running machine? maybe new flag

remote_cache

which default to true and false on some very light processes? 2. Should some process not speculate (local process race with remote cache) because they are too heavy to start? I'm thinking mainly nailgun, java processes or tests.

1. Some processes should not be remote cached? like finding binaries on the running machine? maybe new flag

remote_cache

which default to true and false on some very light processes?

yea, this would be a useful optimization. currently these processes are marked as “per session”, but the implementation of that flag is actually to use some random information that will cause them to miss, rather than actually skipping the cache for them: see https://github.com/pantsbuild/pants/blob/f6b2832539e26ddffb19a7d665b9cef697abd0da/src/rust/engine/process_execution/src/remote.rs#L891-L903

I think it might have very small impact on performance because it takes a slot of the remote cache concurrency. It can also affect the performance of the remote cache server.

So the nailgun process would continue and try to build the canceled process and only if that slot is required would kill it?

that’s right.

in heavy compilation project (we have around 900 coarsened scala targets), I think that solution might be equal to the current state, especially as the next process would start immediately, because of the speculate read.

yea, this is true. #14937 is probably more useful for local iteration, when you’re not compiling your entire graph from scratch. what the optimal strategy is depends a bit on your cache hit rate: • if you’re going to hit for all cache entries, then you didn’t need to speculate at all • if you’re going to miss for all cache entries, then speculating definitely helped

in theory, we can have an algorithm to estimate the cache hit: 1. For leafs in the DAG - assume cache hit 2. For any other node - if all dependencies were cache hit - assume cache hit

i’ll open a ticket about a delay. because “not speculating on nailgun processes” is equivalent to an infinite delay

thanks

thanks!

I'm trying something, will have a draft soon, that the general idea:

in_workunit!(
      "remote_cache_read_speculation",
      Level::Trace,
      |workunit| async move {
        tokio::select! {
          cache_result = cache_read_future => {
            self.handle_cache_read_completed(cache_result).await
          }
          s = tokio::time::sleep(tokio::time::Duration::from_millis(100)) => {
            tokio::select! {
              cache_result = cache_read_future => {
                self.handle_cache_read_completed(cache_result).await
              }
              local_result = &mut local_execution_future => {
                workunit.increment_counter(Metric::RemoteCacheSpeculationLocalCompletedFirst, 1);
                local_result.map(|res| (res, false))
              }
            }
          }          
        }
      }
    ).await
  }

Yea, that should do it too.

almost, I'm getting

error[E0382]: use of moved value: `cache_read_future`
   --> process_execution/src/remote_cache.rs:303:30
    |
298 |           cache_result = cache_read_future => {
    |                          ----------------- value moved here
...
303 |               cache_result = cache_read_future => {
    |                              ^^^^^^^^^^^^^^^^^ value used here after move
    |

the first one can maybe be:

cache_result = &cache_read_future => {

cool, that helped. Now I have:

Compiling process_execution v0.0.1 (/Users/somdoron/git/pants/src/rust/engine/process_execution)
error[E0277]: `&Pin<Box<dyn futures::Future<Output = std::option::Option<FallibleProcessResultWithPlatform>> + std::marker::Send>>` is not a future
   --> process_execution/src/remote_cache.rs:293:9
    |
293 | /         tokio::select! {
294 | |           cache_result = &cache_read_future => {
295 | |             self.handle_cache_read_completed(workunit, cache_lookup_start, cache_result, local_execution_future).await
296 | |           }
...   |
307 | |           }
308 | |         }
    | |_________^ `&Pin<Box<dyn futures::Future<Output = std::option::Option<FallibleProcessResultWithPlatform>> + std::marker::Send>>` is not a future

thanks, have a good night!

ahh, amazing! I will add the configuration part from the process tomorrow, that should be easy.

thank you!

I'm getting very good result with this, running without local cache or pantsd, fully cached build from remote cache in 25 seconds.

Awesome to hear: thanks a lot for the patches to make it happen!

Woot!