CI: Faster GetReleaseVersion and Release/Upload Artifacts jobs #68
Conversation
This file contains 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
Backslashes ("\") can be interpreted as escape characters
on Unix (Linux, macOS). Replace with forward-slashes, "/",
which are interpreted the same (as directory separators) on all OSes,
at least in arguments to commands such as `cd` and `node`.
Ubuntu is much faster than Windows at installing many small files, such as during `git clone` and `npm install`.
This should be marginally faster. No need to run on Windows.
DeeDeeG
changed the title
aminya
reviewed
Jul 26, 2020
script/vsts/nightly-release.yml
Outdated
Comment on lines
4
to
12
| vmImage: 'windows-latest' | ||
| vmImage: 'ubuntu-latest' | ||
| steps: | ||
| # This has to be done separately because VSTS inexplicably | ||
| # exits the script block after `npm install` completes. | ||
| - script: | | ||
| cd script\vsts | ||
| npm install | ||
| cd script/vsts | ||
| npm install request-promise-native yargs | ||
| displayName: npm install | ||
| - script: node script\vsts\get-release-version.js --nightly | ||
| - script: node script/vsts/get-release-version.js --nightly |
There was a problem hiding this comment.
Could you make these a template? We can do it in a separate PR too. No difference.
There was a problem hiding this comment.
Sure thing!
Separate PR strikes me as the right approach. Easier to look back at the history and see what happened.
Will do it right away though after/if this gets merged.
Edit: Issue for this: #69
|
Thanks. It is twice faster 😄 🎉 |
This comment has been minimized.
This comment has been minimized.
DeeDeeG
force-pushed
the
faster-GetReleaseVersion-and-Release
branch
from
7af6423
to
ef6f0ca
Compare
`npm ci` doesn't try to reconcile package.json with package-lock.json, nor with any existing packages in `node_modules`. `npm ci` simply deletes `node_modules` and uses the packages from `package-lock.json`. As a result, `npm ci` is much, much faster than `npm install`. We should use it wherever possible.
DeeDeeG
force-pushed
the
faster-GetReleaseVersion-and-Release
branch
from
ef6f0ca
to
dd5d536
Compare
|
@aminya this is ready to go if you are okay with it. |
|
Going to merge this some time tomorrow if I don't hear back. It's a tiny change, and it's working great. |
This was referenced Jul 30, 2020
|
Upstream PR: atom#21147 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Requirements for Contributing a Performance Improvement
Description of the Change
Run GetReleaseVersion (the first step) and the "Release"/"Upload Artifacts" (the last step on the Nightly/Release Branch pipelines) on
ubuntu-latest. This alone has the most profound performance impact.Beyond that, further tweak GetReleaseVersion to use
npm cirather thannpm install. This saves up to about 25 to 40 seconds or so per run.In order to run these jobs of CI on Ubuntu, backslashes
\were converted to forward-slashes/. This has the side benefit that these jobs may now be run on any OS, should it be desirable to do so in the future.Background on slashes ("\" and "/"):
The
cdandnodecommands each perfectly understand forward-slashes/to be directory separators, if used in command-line arguments, regardless of OS (yes, even on Windows). There is no need to use Windows-only backslashes\, which are interpreted as escape characters on Unix-likes such as Ubuntu and macOS. As it turns out, we only ever use backslashes\in paths passed as command-line arguments tocdornode. So in all of our CI configs, it is never required that we use backslashes\, and we could replace all of them with forward-slashes/if we wanted to. I have left the conditional, Windows-only ones in for now, since they aren't hurting anything.)Quantitative Performance Benefits
About 30 seconds to 1 minute faster GetReleaseVersion (the very first stage of CI, which must finish for the rest of CI to happen).
Presumably similar impact to the final "make releases"/"upload artifacts" steps of CI.
Edit to add some informal "benchmarks" I ran for just the GetReleseVersion job:
https://dev.azure.com/DeeDeeG/b/_build/results?buildId=209&view=results
From fastest to slowest:
I ran seven runs of this (again, informal benchmarks) and results were usually within those ranges, with some slower outliers.
Runs might be a bit faster on my personal fork with fetch-depth set to
7for the implicitgit checkouttask. TheatomcommunityAzure Pipelines project currently has it set to a more-conservative depth of15.Possible Drawbacks
None that I can think of. A pretty small and simple change for a minute or so of speedup.
Verification Process
Passed CI on my personal fork. Should pass CI here.
Compare runs before/after this PR to see that the typical GetReleaseVersion job is faster and takes a more consistent amount of time. With this PR, the GetReleaseVersion job lasts about 30 seconds or so every time. Before this PR, GetReleaseVersion lasted from about 1 minute to 1 minute 30 seconds.
I haven't compared the run time of the Release/Upload Artifacts jobs, because we have no passing runs of that. See the fix in #66 so we can start comparing run times for that.
Applicable Issues
#1 (comment)
Release Notes
N/A