-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not run SKIPIF
section of PHPT test in separate process when it is free of side effects
#5998
Conversation
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Blogged about it: https://staabm.github.io/2024/10/19/phpunit-codesprint-munich.html |
…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--`
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.phpt
I can see the following results:macos:
after this PR:
00:00.070 - 00:00.077
before 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.395
before 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