Implement readahead cache for Workspace API calls

[pietern]

Changes

The reason this readahead cache exists is that we frequently need to recursively find all files in the bundle root directory, especially for sync include and exclude processing. By caching the response for every file/directory and frontloading the latency cost of these calls, we significantly improve performance and eliminate redundant operations.

Tests

• Working on unit tests

[shreyas-goenka]

Awesome! The code looks good to me!

We should add some performance regression tests here, to make sure we test and avoid regressions in some of the performance characteristics this cache introduces. Ideas:

1. Cache is used if present, API call is not made again for the same path.
2. ReadDir on a directory leads to recursive queueing / querying of the downstream directories.
3. Error during a recursive ReadDir call, when walking the file tree, propagates upwards. A user of the cache is not stuck indefinitely.
4. Additional Stat API calls are not made for files during a ReadDir file tree walk. DirEntry is reused for the stat calls. Additional calls are only done for notebooks.
5. ReadDir / Stat calls are immediately added to the queue. Adding elements to the queue does not depend on the API calls actually returning.
6. Stat API calls defer to "parent" ReadDir calls when it is already queued.

[shreyas-goenka]

Do you know if this is the primary reason for the performance gain when using this cache? That is we avoid the redundant stat API calls?

[pietern]

Not primary, but it helps. Primary I would say is caching in the first place. The extras are a cherry on top.

[shreyas-goenka]

Given we'll likely migrate over to FUSE in a reasonable timeline, we can likely skip extensive performance coverage for this PR.

[renaudhartert-db]

LGTM, it was an interesting PR to review!

[no action required]

If my understanding is correct, the workers remain alive with the cache (until the Cleanup function is called) no matter if there is work to do or not. This bothered me a little and I was wondering if we could have something that creates the workers on a need basis.

For the records, here's what I came up with:

funcnewWorkspaceFilesReadaheadCache(fFiler) *cache {
c:=&cache{ ... }
ctx:=context.Background()
goc.work(ctx)
returnc
}
func (c*cache) work() {
semaphore:=make(chanstruct{}, 10) // limit the creation of goroutineswg:= sync.WaitGroup{}
fore:=rangec.queue { // stops when Cleanup is calledsemaphore<-struct{}{} // acquire a spot in the semaphorewg.Add(1)
gofunc(eexecutable) {
deferfunc() { <-semaphore }() // release the spot in the semaphoredeferwg.Done()
e.execute(context.Background(), c)
}(e)
}
wg.Wait()
}
func (c*cache) Close() { // previously Cleanupclose(c.queue)
}

The main difference is that we now have one single long-lived worker that "pops" up to 10 sub-workers to help it empty the queue. I like that it moves most of the concurrency management in c.work.

[pietern]

@shreyas-goenka Thanks for the test suggestions. I have a couple in a local copy and will submit a separate PR for them.

@renaudhartert-db Varying the # of goroutines could work, but it doesn't buy much if there is still one active (i.e. it doesn't remove the need to call Cleanup when done). Alternatively, there could be a way to dynamically spawn and terminate workers as work is added to the queue.

I scaled down the # of workers to 1 to remove concerns about parallelism. The net benefit of the cache is still high, even with only a single background worker doing readahead (I suspect parallel requests were hitting the SDK's rate limiter).