Inline SKIPIF evaluation without side-effects #5974
Closed
Conversation
This file contains 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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5974 +/- ##
============================================
- Coverage 94.67% 94.63% -0.05%
- Complexity 6406 6413 +7
============================================
Files 694 694
Lines 19189 19212 +23
============================================
+ Hits 18167 18181 +14
- Misses 1022 1031 +9 ☔ View full report in Codecov by Sentry. |
staabm
force-pushed
the
side-effects
branch
3 times, most recently
from
57034fa to
6f6390e
Compare
clxmstaab
force-pushed
the
side-effects
branch
2 times, most recently
from
1e344b0 to
2f10d5e
Compare
clxmstaab
force-pushed
the
side-effects
branch
from
ebf68db to
4f3636a
Compare
clxmstaab
added a commit
to staabm/patchwork
that referenced
this pull request
Oct 11, 2024
To unlock PHPUnit side-effect-free in-process --SKIPIF-- evaluation, we use regular echo instead of die(). That way PHPunit can detect whether conditions may be evaluated in the main-process to reduce sub-process creation overhead. see sebastianbergmann/phpunit#5974
|
Tested this PR on a real world project with antecedent/patchwork#168 (and described the results in the PR) |
clxmstaab
added a commit
to staabm/patchwork
that referenced
this pull request
Oct 11, 2024
To unlock PHPUnit side-effect-free in-process --SKIPIF-- evaluation, we use regular echo instead of die(). That way PHPunit can detect whether conditions may be evaluated in the main-process to reduce sub-process creation overhead. see sebastianbergmann/phpunit#5974
staabm
commented
Oct 18, 2024
| } | ||
|
|
||
| foreach ($sideEffects as $sideEffect) { | ||
| if ($sideEffect === SideEffect::STANDARD_OUTPUT) { |
There was a problem hiding this comment.
We could even report a SkipIf condition to be broken, when it does not contain a standard-out side-effect
|
todo
|
|
retargeted in #5998 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Proof of concept
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)
Possible future idea
—CLEAN—as long as the code cannot modify the parent process (e.g. file IO is fine within the parent process)I am pretty sure this needs more work but wanted to have opinions first, whether this is acceptable.
refs #5973