Performance Test: Reuse tests-branch build if possible #45737
Merged
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
|
|
|
Size Change: 0 B Total Size: 1.3 MB ℹ️ View Unchanged
|
dmsnell
force-pushed
the
tests/reuse-tests-branch-build-if-possible
branch
2 times, most recently
from
a7fb6c1
to
247c765
Compare
|
Rebased from a7fb6c1 In that one we were using With this change we could miss an opportunity when |
dmsnell
force-pushed
the
tests/reuse-tests-branch-build-if-possible
branch
2 times, most recently
from
8c2b03d
to
4f85b89
Compare
|
Changes look good, I'm curious if you were able to measure the time diff. (I see you mentioned this on the description) |
youknowriad
approved these changes
Nov 17, 2022
When running the performance test suite we currently build three copies of the plugin: one for each branch under test and also one for the branch which provides the actual test suite. In this patch we're checking first if one of the branches under test is the same as that tests branch, and if so, re-using the built files from that to avoid the additional approximately five minutes of building.
dmsnell
force-pushed
the
tests/reuse-tests-branch-build-if-possible
branch
from
4f85b89
to
49d0301
Compare
Yes @youknowriad I was! Surprisingly before the merge of #45761 there was no clear impact on the test runtime from this, but now after 30 runs in this branch against 26 runs in This trend appeared overnight as well in #45284 which also wasn't demonstrating a clear impact before #45761, now showing a very significant reduction in average runtime, and measuring another six minute reduction against It's surprising to me how this has played out but I suppose it hints the theory that context-switching in the e2e perf tests, probably due to async behaviors and Of note, both this PR and the "no build types" PRs operate in the region of the workflow that use all the available CPU cores. The actual e2e test portion drops down to roughly using only a single CPU core. Just tossing this out there, but it could be possible, ironically, that if we parallelize our test branches we may find more consistent results as well as finish quicker. That is, maybe if we're putting pressure on our VM container it will continue to dedicate the resources to us instead of giving them up to someone else. I'll experiment and report on it! |
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.
What?
In #23842 we introduced
--tests-branchto the performance test suite config which lets us run the test files from a separate branch than are found in the branches under test. A side-effect of this is that we have to build Gutenberg not only for each commit under test, but also for the commit containing the test suites. In practice this adds around five or six minutes to the time it takes to run the performance tests workflow.In this patch we're checking first if one of the branches under test is the same as that tests branch, and if so, re-using the built files from that tests branch to avoid the additional re-build of the app. This removes those five or six minutes from each workflow run.
How?
For the sake of keeping the check simple we are comparing the string literal provided to
cli.jsfor identity of the separate branches. Using this naive approach, we could miss some opportunities to avoid re-building the app if, for example, we pass a commit SHA for the branch under test but a branch name for the tests branch.In a7fb6c1f2169a3f1c034a939b7630e0cda37c56f I started by resolving each given reference to a commit SHA because of this very problem, but for now I found that the additional complexity obscured what's going on. In later work I'd like to explore refactoring the test setup to a resolver stage and then a builder stage where we could resolve these lingering issues better, but for now this smaller and less-reliable change should benefit all PR test runs since we currently pass the same literal value for the branch and tests branch -
$GITHUB_SHA.Why?
Performance tests remain the bottleneck for CI test runs and slow down code merges. This change reduces the feedback cycle and hopefully removes some developer frustration while waiting on the tests to complete.