Skip to content

reduce redundant artifact, CAS, and cache-key work #2172

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.

Sign up for GitHub

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

Open
kotborealis wants to merge 3 commits into
base: master
Choose a base branch
from
+5 −5
Open

reduce redundant artifact, CAS, and cache-key work #2172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a5a1e04
_artifact: Remove unused output size traversal
kotborealis Aug 12, 2026
b69c683
_cas: Avoid duplicate directory tree fetch
kotborealis Aug 12, 2026
857681c
element: Reuse build dependencies for cache keys
kotborealis Aug 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/buildstream/_artifact.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,6 @@ def cache(

context = self._context
element = self._element
size = 0

filesvdir = None
buildtreevdir = None
Expand All @@ -247,7 +246,6 @@ def cache(
filesvdir = CasBasedDirectory(cas_cache=self._cas)
filesvdir._import_files_internal(collectvdir, properties=properties, collect_result=False)
artifact.files.CopyFrom(filesvdir._get_digest())
size += filesvdir._get_size()

with tempfile.TemporaryDirectory() as tmpdir:
files_to_capture = []
Expand Down
2 changes: 1 addition & 1 deletion src/buildstream/_cas/cascache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ def fetch_directory(self, remote, dir_digest):
"Failed to fetch directory tree {}: {}: {}".format(dir_digest.hash, e.code().name, e.details())
) from e

required_blobs = self.required_blobs_for_directory(dir_digest)
required_blobs = self.required_blobs_for_directory(dir_digest, _fetch_tree=False)
self.fetch_blobs(remote, required_blobs)

# pull_tree():
Expand Down
6 changes: 4 additions & 2 deletions src/buildstream/element.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3337,6 +3337,8 @@ def __update_cache_keys(self):
# 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))

@nathanwilliams-ct nathanwilliams-ct Aug 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

@kotborealis kotborealis Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@nathanwilliams-ct nathanwilliams-ct Aug 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to open a follow-up issue for this then and we can close this thread?

@kotborealis kotborealis Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2176


if self.__weak_cache_key is None:
# Weak cache key includes names of direct build dependencies
# so as to only trigger rebuilds when the shape of the
Expand All @@ -3353,14 +3355,14 @@ def __update_cache_keys(self):
if self.BST_STRICT_REBUILD or e in self.__strict_dependencies
else [e.project_name, e.name]
)
for e in self._dependencies(_Scope.BUILD)
for e in build_dependencies
]
self.__weak_cache_key = self._calculate_cache_key(dependencies)

context = self._get_context()

# Calculate the strict cache key
dependencies = [[e.project_name, e.name, e.__strict_cache_key] for e in self._dependencies(_Scope.BUILD)]
dependencies = [[e.project_name, e.name, e.__strict_cache_key] for e in build_dependencies]
self.__strict_cache_key = self._calculate_cache_key(dependencies, self.__weak_cache_key)

if self.__strict_cache_key is None:
Expand Down