Do not run SKIPIF section of PHPT test in separate process when it is free of side effects#5998
Merged
sebastianbergmann merged 1 commit intosebastianbergmann:11.5from Oct 18, 2024
Merged
Conversation
staabm
commented
Oct 18, 2024
Comment on lines
+275
to
+280
| <copy file="${basedir}/vendor/staabm/side-effects-detector/LICENSE" tofile="${basedir}/build/tmp/phar/staabm-side-effects-detector/LICENSE"/> | ||
| <copy todir="${basedir}/build/tmp/phar/staabm-side-effects-detector"> | ||
| <fileset dir="${basedir}/vendor/staabm/side-effects-detector/lib"> | ||
| <include name="**/*.php" /> | ||
| </fileset> | ||
| </copy> |
Contributor
Author
There was a problem hiding this comment.
is this correct like that?
2697a8f to
a39878b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 11.5 #5998 +/- ##
============================================
+ Coverage 94.76% 94.78% +0.01%
- Complexity 6807 6814 +7
============================================
Files 720 720
Lines 21603 21626 +23
============================================
+ Hits 20473 20498 +25
+ Misses 1130 1128 -2 ☔ View full report in Codecov by Sentry. |
cd00575 to
ad2aaeb
Compare
SKIPIF section of PHPT test in separate process when it is free of side effects
Contributor
Author
|
Blogged about it: https://staabm.github.io/2024/10/19/phpunit-codesprint-munich.html |
This was referenced Oct 27, 2024
xabbuh
added a commit
to symfony/symfony
that referenced
this pull request
Oct 29, 2024
…staabm) This PR was merged into the 7.2 branch. Discussion ---------- [PhpUnitBridge] Don't use `die()` in PHPT `--SKIPIF--` | Q | A | ------------- | --- | Branch? | 7.2 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | -- | License | MIT Unlocks a performance optimization in PHPUnit 11.5.x [PHPUnit 11.5.x will be able to avoid subprocess creation](sebastianbergmann/phpunit#5998), when `--SKIPIF--` code in PHPT tests does not `exit()` or `die()` and the logic is side-effect free (output is allowed). more details in https://staabm.github.io/2024/10/19/phpunit-codesprint-munich.html Commits ------- 83b09ba Don't use `die()` in PHPT `--SKIPIF--`
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 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.
while looking into PHPT performance, I realized that the
--SKIPIF--condition is evaluated in a separate subprocess.this means a major overhead for a maybe pretty simple side-effect-less condition like:
I built a small class in which you can pass a string of source-code and it returns whether the given code would have side-effects when evaluated.
In PHPUnit I utilize the detected side-effect-free SKIPIF conditions and evaluate such code in the main-process instead of creating a new one. In case we cannot say with 100% certainty the code does not have side-effects we play save and run it in the subprocess.
when running
php ./phpunit tests/end-to-end/event/assert-failure.phptI can see the following results:macos:
after this PR:
00:00.070 - 00:00.077before this PR:
00:00.092 - 00:00.098-> which means ~20% faster on my mac m1pro on a single test-case.
on windows with
after this PR:
00:00.360 - 00:00.395before this PR:
00:00.406 - 00:00.443-> seems also roughly ~20% faster
(see how slow running the same test is on windows vs. macos)
refs #5973