-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add #[YieldReady]
to allow extensions to tell when they're ready for yielding
#3999
Conversation
047ec50
to
1e48731
Compare
ab91e9a
to
55129da
Compare
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.
What is the performance impact of the runtime check combined with yield ?
55129da
to
3d3caec
Compare
Note that an effect of this change is that support for Fiber-based runtimes will become possible only as of Twig 4, even if all extensions are yield ready, as there is no way anymore to opt-in for the yield-only mode that unlocks that usage. |
95c2801
to
61c7afe
Compare
#[YieldReady]
on nodes#[YieldReady]
to allow extensions to tell when they're ready for yielding
I did a micro-benchmark comparing the time it takes to call ob_get_length() vs the time it takes to call an empty
I improved this by adding the "use_yield" flag back. When it's turned on, the node visitor will throw if any nodes are not |
d03553d
to
285f863
Compare
PR ready (failures are false positives) |
0fc9739
to
03b884c
Compare
All comments addressed. |
03b884c
to
01f339c
Compare
01f339c
to
998de12
Compare
@@ -82,7 +84,7 @@ public function __toString() | |||
public function compile(Compiler $compiler) | |||
{ | |||
foreach ($this->nodes as $node) { | |||
$node->compile($compiler); | |||
$compiler->subcompile($node); |
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.
Should be reverted
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.
We'd better not: this line allows the compiler to accurately tell which node is leaking via "echo". If I remove it, the error/deprecation message will report about the wrong node (the wrapping one).
Can't we keep this line? Why if not? Calling subcompile instead of compile looks OK to me also, semantically speaking.
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.
note that $compiler->subcompile($node);
is already calling $node->compile($compiler);
if ($this->env->isDebug()) { | ||
ob_start(); | ||
} else { | ||
ob_start(function () { return ''; }); |
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.
You've removed this "feature". I'm fine as we won't be able to replicate it with yield, but that should be noted in the CHANGELOG.
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.
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.
I updated the changelog to tell about what this PR does, but about this specific behavior, I can't figure out what to write. It's too complex to describe to really be a "feature".
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.
Maybe CoreExtension should keep that logic in its captureOutput
. The issue of not leaking output buffers in case of fatal errors still exists when this BC layer is used.
998de12
to
e707774
Compare
Thank you @nicolas-grekas. |
This PR was merged into the 5.4 branch. Discussion ---------- [TwigBridge] Fix compat with Twig v3.9 | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | - | License | MIT This PR relies on twigphp/Twig#3999 Commits ------- f563a4c [TwigBridge] Fix compat with Twig v3.9
This PR was merged into the 5.4 branch. Discussion ---------- [TwigBridge] Fix compat with Twig v3.9 | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | - | License | MIT This PR relies on twigphp/Twig#3999 Commits ------- f563a4c5a8 [TwigBridge] Fix compat with Twig v3.9
…colas-grekas) This PR was merged into the 3.x branch. Discussion ---------- Swap BC layer for yield-ready and reclaim perf loss Follows #3999 Fix #4146 Fix #4103 When `use_yield` is set to false (the default), this PR reverts the implementation of the `render()` method to use a wrapping output buffer instead of hooking between each steps of generators. In this mode, the behavior of the yield method is not "pure": it triggers a mix of yield and echo. But this is fine for render and display methods. When `use_yield` is set to `true`, we skip that wrapping output buffer. This makes twig compatible with fibers (and this also makes compilation fail if a non-YieldReady extension is found.) That makes the name of the option not ideal, but BC rulez FTW. Commits ------- 5d1a19a Swap BC layer for yield-ready and reclaim perf loss
Performance regression fixed in #4216 |
The current opt-in based on "use_yield" doesn't work, because there is no decider to enable the flag:
the flag can be turned on only when all extensions are compatible, and this is fragile, because extensions currently don't have a way to signal that they are ready for yielding.
This PR takes the following approach instead:
#[YieldReady]
. If they don't, a deprecation is raised.On Twig 4, we could then just remove this output buffer check and possibly throw when a node is not "YieldReady".
(Classes that extend
AbstractExpression
are implicitly considered "YieldReady".)Best reviewed ignoring whitespaces.