reduce redundant artifact, CAS, and cache-key work - #2172
Conversation
| # This code can be run multiple times until the strict key can be calculated, | ||
| # so let's ensure we only ever calculate the weak key once, even though we need | ||
| # to resolve it before we can resolve the strict key. | ||
| build_dependencies = list(self._dependencies(_Scope.BUILD)) |
There was a problem hiding this comment.
Just to note:
I've proposed a refactor of _calculate_cache_key over here: c3369b7#diff-7d3ea8e226c37028881ae2f47facfe404a1c84346d4238569a63b0caacfb0ea2R2360 To avoid the complicated list comprehension that goes on here, with it's mess of dynamic typing of list[tuple[str,str] | tuple[str,str,None] | tuple[str,str,str]] and hard to read logic.
Although, I still end up calling _dependencies every time _calculate_cache_key is called...
Feedback:
hmm.. Instead of storing the result of _dependencies here, it might be worth refactoring _dependencies to directly cache it's own results, so it can return the cached result after the first call, whenever it's called anywhere instead of just here. I don't think __build_dependencies or __runtime_dependencies is ever updated after an Element is first initialised in _new_from_load_element. (Although _add_build_dependency might be a problem? that would need investigating, but you could clear the cache, in that method..)
...
__dependency_set_cache: dict[str,set[Element]] = {}
def __dependencies(...) -> ...:
# Check if we already calculated this set of dependencies
dependency_set_cache_key = f"{scope}, {recurse}" # (probably need to include the _dependencies 'visited' argument here too...)
if (dependencies := self.__dependency_set_cache.get(dependency_set_cache_key)) is not None:
for element in dependencies:
yield element
return
# Calculate result/visited
....
# Store result for next time
self.__build_dependencies_set[dependency_set_cache_key] = result # ( or 'visited' from the recursive visit function)It will need some thinking, but maybe it would even be appropriate to use functools @cache annotation? https://docs.python.org/3/library/functools.html The recursive visit function might also benefit from the @cache annotation..
There was a problem hiding this comment.
I agree that caching _dependencies() could provide a better optimization, but for this PR I kept the change deliberately narrow.
A cache would need careful invalidation when dependencies are added through.
I would prefer to handle that broader refactoring separately, with dedicated tests. The current change only targets the duplicate traversal.
There was a problem hiding this comment.
Do you want to open a follow-up issue for this then and we can close this thread?
|
The failed CI jobs are all failing for the same unrelated reason.
|
#2175 Fixes this. |
PR merged, you should be able to rebase the branch for a happy CI. |
Artifact.cache() already stores the collected directory digest and never uses the recursive size calculation that follows it. Remove the dead accumulator and _get_size() call so artifact creation does not walk the output tree a second time.
fetch_directory() has already fetched the directory protos with FetchTree before enumerating required blobs. Tell required_blobs_for_directory() to reuse those local protos instead of issuing another FetchTree request, reducing remote CAS round trips.
Weak and strict cache keys both consume the same build dependency list. Materialize the generator once and reuse it so cache-key updates do not traverse the dependency graph twice while preserving the existing key inputs and ordering.
dc65f9c to
857681c
Compare
2 participants
This change removes redundant work from artifact creation, remote CAS directory fetching, and cache-key calculation:
FetchTreerequest inCASCache.fetch_directory().