-
Notifications
You must be signed in to change notification settings - Fork 99
improved caching #551
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.
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
improved caching #551
Conversation
Signed-off-by: Akarsh Sahlot <[email protected]>
9aed00f to
68ddabe
Compare
smolkaj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we adding a LICENSE file and a dubious README..md file to the workflows folder?
deleted unnecessary files Signed-off-by: Akarsh Sahlot <[email protected]>
deleted unnecesaary file Signed-off-by: Akarsh Sahlot <[email protected]>
deleted unnecessary file Signed-off-by: Akarsh Sahlot <[email protected]>
If I understand correctly, the LICENSE and README.md file might be from a software package that helps one run Github Actions workflows on a local development system. If so, I agree that they should be removed from this PR, leaving only the modified ci-build-proto.yml file (which I have not attempted to understand or review the changes proposed here). |
deleted read me file Signed-off-by: Akarsh Sahlot <[email protected]>
|
| strategy: | ||
| matrix: | ||
| # We only test on the oldest version we want to support and latest. | ||
| # We trust that things also work for versions in the middle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing these comments seems like the wrong thing to do, unless you know that they are obsolete or wrong. If they are correct, they were written by someone who wanted to remember the reason that the lines of code were added, and why they were written in this way. That is valuable information, and should not be discarded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I will add them back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are many more such comments removed elsewhere in your PR. My comment applies to all of those changes, not only the ones that I directly commented on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, please look at the diffs of your PR on the Github web site to see if it is what you think it should be. Without doing that, you might be proposing changes in the PR that you forgot you made, or never even intended to make.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I will add back all those changes in comments
added back uncommented part Signed-off-by: Akarsh Sahlot <[email protected]>
|
@AkarshSahlot thanks for looking into improving cache performance! Fast CI builds are super useful to allow quickly operating on PRs when they don't build the first time around. From what I understand, the main improvement here is to enable Bazel disk caching? A few questions:
|
|
@AkarshSahlot bump? |
|
Closing this PR since it currently seems to be stale. |
Logic for caching prior is:
Choose a completely new entry in the cache only if:
The build is taking place on the primary branch,
Build duration is more than 3 minutes (env.duration > 180),
The step is executed self-sufficiently (always()).
The updated version improved the cache by:
Efficient use of bazel-disk caching to improve performance.
Broadening the restore-keys to increase the possibility of cache hits.
Performing simultaneous builds to reduce the total build time.
overall speedup bazel CI builds