Repository navigation
ci: run the race step in the checks job - #1387
Open
fuad-daoud wants to merge 4 commits into
Open
fuad-daoud wants to merge 4 commits into
fuad-daoud wants to merge 4 commits into
Conversation
The suite job ran coverage and then the race detector, one after the other, while the checks job finished well before it, so suite alone set how long a pull request waited. The race step now runs in checks, after vet, lint, the planted lint findings, the fuzz target checks and the benchmarks. It comes after them because each of those steps runs git, and a git call after the dating gives .git a new time. The race packages do not hash it today, but the tests of the suite job do, and a package added to the race list could. checks dates files by content before the race step and, on main, clears the restored test results first, as suite does before coverage. Its save then carries the race results a pull request reuses. No make build comes first: after make vet, go list -export compiles nothing the race packages' tests would need. The job ids, and with them the cache keys, are unchanged. Only the display names move: "lint, bench, race detector" and "build, coverage (exactly 100%)".
The comments over the dating and race steps in the checks job recorded probe results: compile counts from go list -export, and the fact that no race package lists the module root. Nothing derives either one, so a change to the module or the race list would leave them wrong with the gate green. The comments now give only the reasons. The dating comment also claimed that every step above it runs git. The toolchain pin, the cache restore and the lint version steps do not, so it now names the steps that do.
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
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.
Summary
The
suitejob ran coverage and then the race detector, one after the other, while thechecksjob finished well before it, sosuitealone set how long a pull request waited. On the four runs of #1382 and #1385,suitetook a median of 7.4 min againstchecks' 3.3. The race step now runs inchecks, after vet, lint, the planted lint findings,fuzz-checkand the benchmarks. That gives the two jobs about the same length and keeps the gate at two jobs.checksgains what the race step needs to reusemain's results:make filetimesbeforemake race, assuitedates files before coverage, so Go's test cache survives the checkout.go clean -testcacheonpushbeforemake race, somainreruns every race test rather than replaying a pass it stored.The two steps are YAML anchors in
checks, andsuitereuses them by alias.make buildinchecks. Some race packages' tests load packages throughgo list -exportbeside tests that time a child process (test: child-process probes read a child slowed by load as hung #1010), which is whysuitebuilds first. Aftermake vet, that load compiles nothing (counts below), sochecksneeds no build step.git. Each of those steps runsgit:make fmt, the scripts'rev-parse, and the VCS stamping ingo buildandgo list. No race package lists the module root today, so agitcall after the dating would cost the race step nothing: a mutant that runsmake fuzz-checkbetween the dating andmake raceleaves every race package(cached). The order still matters for the coverage step'sirtestandarchtest, and it costs nothing, so a race package added later that lists the root still hits the cache. The comment over the dating step says so.go-${{ github.job }}-…) still find whatmainsaved. Once race runs inchecksbefore its save,main'scheckssave carries the race results.lint, benchbecomeslint, bench, race detector, andbuild, coverage (exactly 100%), race detectorbecomesbuild, coverage (exactly 100%). No ruleset or branch protection requires a check by name: the ruleset's rules aredeletion,non_fast_forward,pull_requestandcode_quality, andmainreturns404 Branch not protected.Stacked on #1385, which is stacked on #1382, #1373 and #1311; the diff here shows only this change. Once #1385 merges, this is retargeted to
main, since the repository does not delete merged branches.Before and after
flowchart LR subgraph before["Before"] direction TB b1["suite: build → date files → clear on main<br/>→ coverage → race"] --> b3["a pull request waits for suite"] b2["checks: vet → lint → fuzz-check → bench<br/>then idle"] --> b3 end subgraph after["After"] direction TB a1["suite: build → date files → clear on main<br/>→ coverage"] --> a3["the two jobs end about together"] a2["checks: vet → lint → fuzz-check → bench<br/>→ date files → clear on main → race"] --> a3 end before ~~~ afterWhat it brings: a pull request waits for the longer of two balanced jobs instead of coverage and race in a row. #1386 estimates about 2 min off a cold run and off a change to
ir, which reaches nearly every package, and up to about 1 min off a small change, whose race results are mostly reused already. Those are estimates. This pull request's first run, cold like every run of the stack so far, measured it once: run 41594716198 oneba1eaabtook 328s inchecks(vet 132, lint 62, race 111) and 273s insuite(build 53, coverage 204), so 5.5 min, against 8.2 min for run 41594561211 on #1385's head0780cc44with the samemain(suite491s,checks195s). That is one run on each side, and runs on the same code vary widely.Test plan
Measured on a local 8-core, 16-thread AMD Ryzen 7 5800X at
3e5c073e, the change's first commit; the second commit changes only comments.actionlint1.7.12 andyq .are clean ongate.yml.yqlists the steps:checksends in benchmarks → date files → clear (push) → race → save (push), andsuiteholds no race step.make -n raceis byte-identical before and after, and the race step's YAML is the same{"name":"race","run":"make race"}.checks' steps run in order on an empty cache,go list -x -export -depsoverir,engineandcompilers/openapi/internal/loadcompiled 0 packages afterwards.ir, it compiled 29 beforemake vetand 0 after.gitbetween dating and race: a logginggitwrapper onPATHrecordsgit statusfrommake vet,ls-filesfrommake fmt, andrev-parsefrom the scripts. Betweenmake filetimesand the race tests it records nothing. The only call after them israce-reach.sh'srev-parse, once the package tests have returned.GOCACHE:(cached);go clean -testcache: none cached.gitafter the dating is equivalent for today's race list (above).make gateon that machine at both commits, green.After merge:
mainsaves acheckscache with race results.(cached)for the race packages its change does not reach.gh api repos/dexpace/morphic/actions/caches --jq '.actions_caches[] | [.key,.size_in_bytes] | @tsv'gives the size ofgo-checks-*againstgo-suite-*.Closes #1386
Related: #1268, #1010, #1311, #1373