nathanwilliams-ct commented on code in PR #2172:
URL: https://github.com/apache/buildstream/pull/2172#discussion_r3795555037
##########
src/buildstream/element.py:
##########
@@ -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))
Review Comment:
Just to note:
I've proposed a refactor of _calculate_cache_key over here:
https://github.com/apache/buildstream/pull/2167/changes/c3369b7577d9ea32862512c9b38c844cbad38bc5#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..)
```python
...
__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..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
