Improves CI performance - #22995
Conversation
c4f48b7 to
6ce446c
Compare
87c7e25 to
541ac21
Compare
4010558 to
b9e37cf
Compare
5f23b04 to
70b3bec
Compare
70b3bec to
9de2a2e
Compare
6a1ba96 to
2fb2a6e
Compare
2fb2a6e to
09834c1
Compare
6ac7397 to
ae7dcec
Compare
ae7dcec to
1c8ede1
Compare
|
@arnaud-lb maybe it is worth prioritising this one before the others. Would give me a much faster feedback loop for all the rest that likely will receive some more pushes if there is feedback from you to address. Will you look at this one too or should I mark as ready to ping the reviewers? |
arnaud-lb
left a comment
There was a problem hiding this comment.
Nice! Could you split this PR into multiple ones? Each commit seems pretty independent and could be a separate PR.
| !if "$(PARALLEL_BUILD)" == "yes" | ||
| ext\json\php_json_scanner_defs.h: ext\json\json_scanner.c | ||
| ext\json\json_scanner.c: ext\json\json_scanner.re ext\json\json_parser.tab.h | ||
| $(RE2C) $(RE2C_FLAGS) -t ext/json/php_json_scanner_defs.h -bci -o ext/json/json_scanner.c ext/json/json_scanner.re | ||
|
|
||
| ext\json\json_parser.tab.h: ext\json\json_parser.tab.c | ||
| ext\json\json_parser.tab.c: ext\json\json_parser.y | ||
| $(BISON) $(BISON_FLAGS) --defines -l ext/json/json_parser.y -o ext/json/json_parser.tab.c | ||
| !else | ||
| ext\json\json_scanner.c ext\json\php_json_scanner_defs.h: ext\json\json_scanner.re ext\json\json_parser.tab.h | ||
| $(RE2C) $(RE2C_FLAGS) -t ext/json/php_json_scanner_defs.h -bci -o ext/json/json_scanner.c ext/json/json_scanner.re | ||
|
|
||
| ext\json\json_parser.tab.c ext\json\json_parser.tab.h: ext\json\json_parser.y | ||
| $(BISON) $(BISON_FLAGS) --defines -l ext/json/json_parser.y -o ext/json/json_parser.tab.c | ||
| !endif |
There was a problem hiding this comment.
It's unfortunate that we have to maintain two sets of rules for jom/nmake in some cases. Would it be possible to write a set of rules that works for both? Using a stamp file might work:
ext\json\json_scanner.c ext\json\php_json_scanner_defs.h: ext\json\json_scanner.stamp
ext\json\json_scanner.stamp: ext\json\json_scanner.re ext\json\json_parser.tab.h
$(RE2C) $(RE2C_FLAGS) -t ext/json/php_json_scanner_defs.h -bci -o ext/json/json_scanner.c ext/json/json_scanner.re
echo done > ext\json\json_scanner.stamp
| if "%CLANG_TOOLSET%" equ "1" goto build_clang | ||
|
|
||
| sccache --zero-stats | ||
| jom /NOLOGO CC="sccache cl.exe" |
There was a problem hiding this comment.
Nice! I didn't know jom, but it seems to be a drop-in replacement for nmake with parallel execution support. There is at least one previous effort to parallelize windows builds but it was abandoned. Linking it here for reference: GH-17445. I see that you address some of the issues mentioned in the other issue.
There was a problem hiding this comment.
Wasn't aware. Is this just for reference or is there anything specific I should look at in the linked PR?
There was a problem hiding this comment.
Mostly for reference so that we can find related work from either of these PRs, but also to point at #17445 (comment) which mentions some issues that may be relevant here too. I believe that you've addressed at least the PDB one.
|
Thanks for prioritising reviewing this one!
Did you see how many PRs I have open? I am loosing overview, and would kindly ask to not split again here. 😅 All commits together solve one problem. They depend on each other to get the total CI run time balanced. Each commit is reviewable on it's own easily. Please let's not split this just for the sake of it. 🙏 |
|
IMO, there are some useful things in this PR, and some less useful things.
|
not sure I understood this 100%, but from my understanding github actions allows to re-use artifacts from other build jobs using |
|
Thanks for the review @iluuu1994
Wasn't aware of this PR. I am not sure I am following, "too many cache entries were pushed" -- do you mean that also from a resource usage point of view? Or what? My first impulse actually is that what we do here further optimises what you intended? Though, I might misunderstand. An alternative could be to keep the (easy stale going) weekly cache dropped, but not additionally have PR caches, so that PR always use fresh master caches. Would this address what you have in mind? I could benchmark how much slower it is.
That's a trade off I actively decided for because the repo is on the free plan. So as long as we we don't pay for it I'd argue it's good. The build step is not the most expensive. Everyones time is also a resource. :) If we ever switch to bigger runners it would anyway be a different story and we would have other possibilities opening up, and then rethink things? It's not that we take an irreversible decision if we decide to currently benefit from the speedup. |
ASAN builds are so big that I assume sending things around, incl. compressing, uploading, downloading, and extracting would bring back a relevant performance hit. Do you have any experience with that? Any input welcome! |
Effectively, because cache entries from old commits are not removed, cache entries accumulate very quickly and evict relevant cache entries from other branches (there's a 10 GB limit), making the cache completely ineffective.
I do think we should be mindful of how much energy we're wasting. This is adding 5 build steps, so not trivial. If we can re-use the artifacts, then maybe something to consider.
Possibly. Do you know if this will further put pressure on the cache, or can these artifacts be made temporary for just the current workflow run? We'd also need to make sure the jobs run a compatible image (e.g. when the runner image is partially rolled out). |
shivammathur
left a comment
There was a problem hiding this comment.
-
The refactor to add build_with_test_database_setup and setup_test_databases scripts needs to be a separate PR targetting PHP-8.2, otherwise the nightly workflow would fail when this is merged.
-
This increases cache saves from weekly to per commit and will result in a lot of cache evictions, as @iluuu1994 mentioned. Also, stable-branch builds use the master commit SHA in their cache keys instead of the checked-out branch SHA.
sccache currently does nothing for Windows ASAN builds: all compiler calls are reported as non-cacheable. I would say sccache can be skipped for Windows ASAN unless fixed. -
Can you check whether
--enable-parallel-buildwith jom supports incremental builds? It would be useful for local builds as well. Currently, it appears that a second build without any changes would rebuild all objects. -
The
--enable-parallel-buildargument says that it supports a parallel NMAKE-compatible tool, but since this PR only supports jom, it would be better to be clear. -
It might be better to have
jomas part of binaries in php/php-sdk-binary-tools instead of fetching it separately on each CI run.
|
Thanks for the review @shivammathur! I'll wait a bit longer to see whether more feedback comes in, and then address and look into everything in one go. 🫡 |
tl/dr: ~45m -> ~22m; this is CI-only stuff -- everything in #22917 reduces further.
Results
f47a44e (perf: speed up Windows CI)
Cache hit rate: ~95%.
8e526f1 (perf: speed up benchmark checkout)
Avoids downloading unnecessary file contents. From ~1:11 to ~0:20, which is a 72% improvement.
653fa83 (perf: keep non-Windows compiler caches fresh)
Before: the older the weekly cache, the lower the hit rate. 2026-07-27 was ~99% (0:30s) while 2026-08-02 was only ~25% (6:52). Rolling cache solves that.
Cache hit rate: ~99%.
(1) when cache not fresh.
6c933cb (perf: skip unused ASAN setup)
Saves 1:37 per ASAN run. Depending on download speeds shaves off 1–3 min.
a33ae4c (perf: run Function JIT tests concurrently)
Splits from sequentially to separate jobs for normal (16:37m) and JIT (18:47 ); results in 11 minutes earlier finish.
3dcc018 (perf: shard long running jobs)
Critical path dropped from 44:27 to 22:49 (49%). Slowest shards finished in 19:03 for Linux ASAN, 17:39 for Windows x64 ZTS, and 14:43 for Alpine ASAN.
58c43d5 (perf: aded circle ci compile caching)
Cache hit rate: ~99%
Warm cache compile down from ~8:17 to ~3:23, which is 59% improvement. Works slightly different than the other caching. Circle isolates caches that belong to external forks. Trusted
masterbuilds roll the source repository cache forward; same-repository PRs can restore it, fork PRs start cold and then roll their own forward.