Skip to content

Improves CI performance - #22995

Open
NickSdot wants to merge 8 commits into
php:masterfrom
NickSdot:perf/ci-build-times
Open

Improves CI performance#22995
NickSdot wants to merge 8 commits into
php:masterfrom
NickSdot:perf/ci-build-times

Conversation

@NickSdot

@NickSdot NickSdot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

tl/dr: ~45m -> ~22m; this is CI-only stuff -- everything in #22917 reduces further.

Results

f47a44e (perf: speed up Windows CI)

Windows x64 Before After Improvement
Setup + build 10:53 3:22 69%
Tests 30:37 14:48 52%
Full job 41:55 18:48 55%

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.

Compile job Before (1) After Improvement
Linux release 7:54 0:28 94%
Benchmark 2:33 0:17 89%
Unit tests 2:06 0:14 89%
Linux ASAN 1:50 0:27 75%
macOS 4:07 1:30 64%
Alpine ASAN 2:37 0:57 64%
Linux x32 2:10 0:57 56%

Cache hit rate: ~99%.
(1) when cache not fresh.

6c933cb (perf: skip unused ASAN setup)

Setup Before After Improvement
Service containers 1:24 0:13 85%
MSSQL setup 0:14 Skipped 100%
Apt dependencies 0:54 0:42 22%
Total targeted setup 2:32 0:55 64%

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)

CircleCI Before Warm cache Improvement
Compile 8:17 3:23 59%

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 master builds roll the source repository cache forward; same-repository PRs can restore it, fork PRs start cold and then roll their own forward.

@NickSdot
NickSdot force-pushed the perf/ci-build-times branch 2 times, most recently from c4f48b7 to 6ce446c Compare August 2, 2026 09:16
@NickSdot
NickSdot force-pushed the perf/ci-build-times branch 2 times, most recently from 87c7e25 to 541ac21 Compare August 2, 2026 10:01
@NickSdot
NickSdot force-pushed the perf/ci-build-times branch 10 times, most recently from 4010558 to b9e37cf Compare August 2, 2026 17:09
@NickSdot
NickSdot force-pushed the perf/ci-build-times branch 3 times, most recently from 5f23b04 to 70b3bec Compare August 2, 2026 19:41
Comment thread .github/matrix.php Outdated
@NickSdot
NickSdot force-pushed the perf/ci-build-times branch from 70b3bec to 9de2a2e Compare August 2, 2026 21:18
@NickSdot NickSdot changed the title Improves CI build performance Improves CI performance Aug 3, 2026
@NickSdot
NickSdot force-pushed the perf/ci-build-times branch from 6a1ba96 to 2fb2a6e Compare August 3, 2026 09:17
@NickSdot
NickSdot force-pushed the perf/ci-build-times branch from 2fb2a6e to 09834c1 Compare August 3, 2026 09:32
@NickSdot
NickSdot force-pushed the perf/ci-build-times branch 3 times, most recently from 6ac7397 to ae7dcec Compare August 3, 2026 15:06
@NickSdot
NickSdot force-pushed the perf/ci-build-times branch from ae7dcec to 1c8ede1 Compare August 3, 2026 15:42
@NickSdot

NickSdot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

@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?

@NickSdot
NickSdot marked this pull request as ready for review August 4, 2026 15:32
@NickSdot
NickSdot requested review from TimWolla and bukka as code owners August 4, 2026 15:32

@arnaud-lb arnaud-lb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Could you split this PR into multiple ones? Each commit seems pretty independent and could be a separate PR.

Comment on lines +1 to +15
!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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't aware. Is this just for reference or is there anything specific I should look at in the linked PR?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@NickSdot

NickSdot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for prioritising reviewing this one!

Nice! Could you split this PR into multiple ones? Each commit seems pretty independent and could be a separate PR.

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. 🙏

@iluuu1994

Copy link
Copy Markdown
Member

IMO, there are some useful things in this PR, and some less useful things.

  • .github/actions/apt-x64/action.yml is good.
  • filter: blob:none is good.
  • .github/actions/ccache/action.yml: The current solution exists specifically because too many cache entries were pushed, see [CI] Fix ineffective ccache #21258.
  • Shards: I dislike the idea of splitting jobs into multiple parts if the entire build step is repeated. In GitLab CI, we used to have a build step and could then copy artifacts to the next stages that could trivially be parallelized. GitHub doesn't work this way, and repeating the build 3 times defeats the purpose. We don't just want shorter parallel builds, we also want less resource usage.
  • Windows: Can't comment much on the Windows stuff, maybe @shivammathur can have a look for those.

@staabm

staabm commented Aug 6, 2026

Copy link
Copy Markdown
Contributor
  • Shards: I dislike the idea of splitting jobs into multiple parts if the entire build step is repeated. In GitLab CI, we used to have a build step and could then copy artifacts to the next stages that could trivially be parallelized. GitHub doesn't work this way, and repeating the build 3 times defeats the purpose. We don't just want shorter parallel builds, we also want less resource usage.

not sure I understood this 100%, but from my understanding github actions allows to re-use artifacts from other build jobs using actions/upload-artifact, actions/download-artifact within a workflow

@NickSdot

NickSdot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

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.

  • Shards: I dislike the idea of splitting jobs into multiple parts if the entire build step is repeated. In GitLab CI, we used to have a build step and could then copy artifacts to the next stages that could trivially be parallelized. GitHub doesn't work this way, and repeating the build 3 times defeats the purpose. We don't just want shorter parallel builds, we also want less resource usage.

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.

@NickSdot

NickSdot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author
  • Shards: I dislike the idea of splitting jobs into multiple parts if the entire build step is repeated. In GitLab CI, we used to have a build step and could then copy artifacts to the next stages that could trivially be parallelized. GitHub doesn't work this way, and repeating the build 3 times defeats the purpose. We don't just want shorter parallel builds, we also want less resource usage.

not sure I understood this 100%, but from my understanding github actions allows to re-use artifacts from other build jobs using actions/upload-artifact, actions/download-artifact within a workflow

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!

@iluuu1994

Copy link
Copy Markdown
Member

I am not sure I am following, "too many cache entries were pushed"

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.

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.

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.

but from my understanding github actions allows to re-use artifacts from other build jobs using actions/upload-artifact, actions/download-artifact within a workflow

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 shivammathur left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NickSdot

  • 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-build with 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-build argument 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 jom as part of binaries in php/php-sdk-binary-tools instead of fetching it separately on each CI run.

@NickSdot

NickSdot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

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. 🫡

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants