Re-use local build cache between dependency look-ups#2717
Merged
premun merged 1 commit intodotnet:mainfrom Jun 28, 2023
Merged
Conversation
Member
Author
|
@riarenas I would appreciate if you could have a look at this one |
| /// <returns>Async task</returns> | ||
| public async Task AddAssetLocationToDependenciesAsync(IEnumerable<DependencyDetail> dependencies) | ||
| { | ||
| var buildCache = new Dictionary<int, Build>(); |
Member
There was a problem hiding this comment.
I am only wondering if asset.BuildId in if (!buildCache.TryGetValue(asset.BuildId, out Build producingBuild)) is unique.. Besides that, seems you found the spot.
Member
Author
There was a problem hiding this comment.
It is the BAR ID of the build that produced the asset. The IDs of the builds should be unique as I believe they come from an auto-increment SQL table column. But these are my guesses, I can confirm.
ghost
approved these changes
Jun 27, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I took a sample problematic scenario from
dotnet/sdkwhich takes a bit over 6 minutes to complete:I noticed we burn a lot of time querying builds while the cache that we use is too local.
I don't want to make the cache even more long-lived than this PR does as I don't understand the life-cycle of the
Remoteclass within Maestro so it would be too risky to store builds indefinitely.However, even this change in this PR showed 5x speed-up to about 1 minute 15 seconds.
I think this change is not very invasive and safe to do so I would start with this and see where it gets us.
My reasoning was that the only input to this call is the ID so it's not like "give me only some assets for this ID" so I think it's safe to cache this API call result.
This could have a lot of perf impact across the board though.
We don't need to merge this yet and I could deploy this branch to staging from a branch and measure the E2E test validation duration. I suspect we might see a speed-up there already.
#2681