Run the benchmark on PHP 7.4 in addition to PHP 8.5 - #6121
Merged
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| - name: "Run phpbench baseline" | ||
| run: "tests/vendor/bin/phpbench run --dump-file=tests/bench/storage/baseline.xml --ansi" | ||
| run: "tests/vendor/bin/phpbench run --dump-file=tests/bench/storage/baseline-${{ matrix.php-version }}.xml --ansi" |
|
|
||
| - name: "Run phpbench test" | ||
| run: "tests/vendor/bin/phpbench run --file=tests/bench/storage/baseline.xml --report=my-report --ansi" | ||
| run: "tests/vendor/bin/phpbench run --file=tests/bench/storage/baseline-${{ matrix.php-version }}.xml --report=${{ matrix.report }} --ansi" |
|
|
||
| - name: "Run phpbench test" | ||
| run: "tests/vendor/bin/phpbench run --file=tests/bench/storage/baseline.xml --report=my-report --ansi" | ||
| run: "tests/vendor/bin/phpbench run --file=tests/bench/storage/baseline-${{ matrix.php-version }}.xml --report=${{ matrix.report }} --ansi" |
| with: | ||
| working-directory: "tests/" | ||
|
|
||
| - uses: ./.github/actions/downgrade-code |
staabm
reviewed
Jul 28, 2026
| @@ -1,4 +1,4 @@ | |||
| <?php | |||
| <?php // lint >= 8.1 | |||
Contributor
There was a problem hiding this comment.
in #6122 I adjusted the RequiredPhpVersionCommentTest which checks the lint comments
| # with an empty baseline and every assertion passes | ||
| - name: "Check that the baseline exists" | ||
| run: | | ||
| test -f tests/bench/storage/baseline-${{ matrix.php-version }}.xml \ |
| - name: "Check that the baseline exists" | ||
| run: | | ||
| test -f tests/bench/storage/baseline-${{ matrix.php-version }}.xml \ | ||
| || (echo "Missing tests/bench/storage/baseline-${{ matrix.php-version }}.xml, download it from the phpbench-baseline-${{ matrix.php-version }} artifact of the Baseline job and commit it." && exit 1) |
| - name: "Check that the baseline exists" | ||
| run: | | ||
| test -f tests/bench/storage/baseline-${{ matrix.php-version }}.xml \ | ||
| || (echo "Missing tests/bench/storage/baseline-${{ matrix.php-version }}.xml, download it from the phpbench-baseline-${{ matrix.php-version }} artifact of the Baseline job and commit it." && exit 1) |
| with: | ||
| working-directory: "tests/" | ||
|
|
||
| - uses: ./.github/actions/downgrade-code |
Reverts 030b59f and the Configurator part of 0587638. In the phar setup the main process does not load the extension and workers get it via -d extension=, so the key differed between them and a single run compiled two containers - undoing 8fd0fda "Do not compile special DI container for workers". The env unsets right above exist to keep main and workers on one container. Nothing in the compiled container depends on the stubs: every shadowed class is a value object or static helper, none is a service, a constructor dependency of one, or named in any conf/*.neon. Once a shadowed class is registered as a service the container does differ, but only by an inert $wiring entry for the stub's native parent class, and main and workers have to share one container anyway - a container that genuinely depended on the stubs cannot be fixed by a cache key. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EeBk5QAUD6JJecNn6RPYBJ
Both the baseline and the test job are matrixed over PHP 7.4 and 8.5. The 7.4 leg downgrades the source code the same way the test workflow does. phpbench 1.2.14 is the last version that supports PHP 7.4, so that leg pins it, and because it does not have the aggregate-preview report that 'my-report' extends, it uses the built-in 'aggregate' report - its mode column already inlines the percent diff against the baseline. PHPBench attributes are replaced with annotations so that RegressionBench is valid PHP 7.4 - tests/bench is not downgraded, and a multi-line attribute is a syntax error there. That also lets parallel-lint check the file again. Data files that cannot be parsed on the PHP version the benchmark runs on are skipped through the '// lint' comment, the same way TypeInferenceTestCase does it. The baseline is now stored per PHP version. baseline-7.4.xml has to be taken from the phpbench-baseline-7.4 artifact and committed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HijxpJYQ5uGfdo6AkXV7TA
The prose in the class docblock mentioned an annotation name, which the annotation reader tried to parse as an actual annotation with a null value. tests/composer.json pins the Composer platform to PHP 8.2, so Composer resolved PHPBench's Symfony dependencies to 6.4 even with --ignore-platform-reqs, and those cannot be parsed on PHP 7.4. Unsetting the platform makes Composer resolve against the real PHP version, which also makes pinning the individual Symfony packages unnecessary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HijxpJYQ5uGfdo6AkXV7TA
The baseline comes from the phpbench-baseline-7.4 artifact of the Baseline job. PHPBench does not fail when the file passed to --file does not exist - it warns and runs with an empty baseline, so every assertion passes and the job is green while testing nothing. The test job now checks for the file first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HijxpJYQ5uGfdo6AkXV7TA
The first committed baseline came from a runner that was twice as fast as the one that recorded baseline-8.5.xml - PHP 7.4 measured 2x faster than PHP 8.5, which is not possible. Every variant of the test job then looked like a +100% regression. The replacement comes from a runner whose <baseline-machine> values match the ones in baseline-8.5.xml. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HijxpJYQ5uGfdo6AkXV7TA
ondrejmirtes
force-pushed
the
bench-php-7.4
branch
from
July 28, 2026 11:29
13718e8 to
dfefbac
Compare
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
3 participants
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.
Both the
baselineand thetestjob are now matrixed over PHP 7.4 and 8.5. The 7.4 leg downgrades the source code the same way thetests-with-old-phpunitjob does (downgrade root +tests/dependencies, then./.github/actions/downgrade-code).What the 7.4 runtime forced:
^8.1. That version has noaggregate-previewgenerator, whichmy-reportextends, so the 7.4 leg uses the built-inaggregatereport instead. Itsmodecolumn already inlines the percent diff against the baseline, so the output is still readable.tests/composer.jsonpins the Composer platform to PHP 8.2, so even with--ignore-platform-reqsComposer resolved PHPBench's Symfony dependencies to 6.4, which cannot be parsed on PHP 7.4 (unexpected 'static'inUnicodeString.php). The other 7.4 workflows never hit this because they only install root dependencies. Unsetting the platform makes Composer resolve against the real PHP version and pick Symfony 5.4 throughout, which also makes pinning individual packages unnecessary.RegressionBench.tests/benchis not part ofbuild/downgrade.php, so it has to be valid PHP 7.4 on its own, and a multi-line attribute is a syntax error there (single-line ones silently become comments and the benchmark would lose its revs/iterations/asserts). This is also why the file was excluded fromparallel-lint— that exclusion is gone now, so the lint job checks it again. The assertion expression has to stay on one line, and no annotation name may appear anywhere else in the docblock: the annotation reader parses prose mentions too.// lintfirst-line commentTypeInferenceTestCaseuses. 10 of the 72 data files skip on 7.4 — the test job reports 62 assertions there and 72 on 8.5.The baseline file is stored per PHP version:
tests/bench/storage/baseline-8.5.xml(renamed frombaseline.xml) andbaseline-7.4.xml(taken from thephpbench-baseline-7.4artifact of the baseline job).Two things worth knowing for the future, both now documented in comments:
--filedoes not exist. It warns and runs with an empty baseline, so every assertion passes and the job is green while testing nothing. The test job checks for the file first.baseline-7.4.xmlcommitted here came from a fast runner — it measured PHP 7.4 as 2x faster than PHP 8.5, and every variant of the test job then looked like a +100% regression. The<baseline-machine>values PHPBench records in each file (md5,file_rw) identify the machine class and should be compared against the currently committed baseline before replacing it.Note that this variance is pre-existing and applies to the 8.5 leg too: both jobs compare a fresh run against a baseline recorded on a different runner, so a bad draw can exceed the 10-50% thresholds. Removing it would mean recording the baseline and running the test on the same machine within one job.
🤖 Generated with Claude Code
https://claude.ai/code/session_01HijxpJYQ5uGfdo6AkXV7TA