Problem/Motivation
From twig we call twig_render_var, which lives in twig.engine.
twig_render_var calls render(), which calls drupal_render(), which calls \Drupal::service('renderer')->render() (which in between calls static::getContainer()->get('renderer')).
That is a lot of function unnecessary function calls and drupal_render is deprecated anyway.
Proposed resolution
- Let render() call \Drupal::service('renderer')->render() directly.
- Inject the renderer service into the twig extension
- Move twig_render_var to the twig extension
- Inline render() code and call it directly (it is highly unlikely render() will change, so code duplication is okay now)
Remaining tasks
- Do it
User interface changes
- None
API changes
- None
Comments
Comment #1
Fabianx CreditAttribution: Fabianx commentedTagged for 'D8 Accelerate Dev Days' sprint
Comment #2
star-szrComment #3
dawehnerThis sounds like a good idea but even if we do that, we should somehow know how much we gain.
Comment #4
fgmMaybe like this ? Informal checks seem to work.
Comment #6
fgmThe first two errors are because the calls to
drupal_render_var()
need to be replaced inTwigEngineTest::testsRenderZeroValue()
. Not sure what our practice is for this : should the test use the renderer service, or mock it and just use a spy to check that the render() method is called. Or .... ?There is an other error in the TwigFilterTest, but it appears in a compiled file, so I'm not sure where it happens, but it looks like this would be because the previous patch did not declare
[$this, 'renderVar']
inTwigExtension::getFilters()
. This reroll should have one less fail.Comment #8
fgmSince this is now a method on the extension, the test can no longer be a unit test, so moved it to the TwigExtension suite. Passes locally.
Comment #9
Fabianx CreditAttribution: Fabianx commentedThis should be inlined as well.
--
Thanks for the work! Looks great already!
Comment #10
fgmDone.
Comment #11
Fabianx CreditAttribution: Fabianx for Acquia commentedThanks this looks great, it would be RTBC, but I want to get some profiling numbers for it - as this could / should save quite some time.
Comment #12
Fabianx CreditAttribution: Fabianx for Acquia commentedOn 2nd look:
Please do the same for twig_drupal_escape_filter(), which also calls 'render()' and has the logic of renderVar inlined for performance reasons.
Comment #13
fgmLet's see if it still works.
Comment #15
fgmForgot to remove the replaced test.
Comment #16
fgmInterdiff between #10 and #15.
Comment #17
dawehnerIt would be nice to explain why we replace it and what is the difference.
Is that copied from the parent or could be throw an invalid argument exception?
Seems a bit of an optimisation for an uncommon case, how often do we print out 0 or 0.0
Comment #18
Fabianx CreditAttribution: Fabianx for Acquia commentedRTBC, but still needs the profiling, will do so later ...
Can we open a follow-up postponed on this issue to move the remaining twig engine functions to the extension? They are not considered public API anyway and having them in the extension is better and more consistent. Also this could allow unit testing of the twig extension in the future.
Comment #19
Fabianx CreditAttribution: Fabianx for Acquia commented#17: This is unfortunately all out of scope here, this is just moving code around.
Comment #20
dawehnerSorry for my comment, didn't thought about too much.
Comment #21
Fabianx CreditAttribution: Fabianx for Acquia commentedQuick IS update
Comment #22
Fabianx CreditAttribution: Fabianx for Acquia commentedHere is the report:
For 50 nodes with render cache off, we save 1% or 2836 function calls with xhprof on.
Percentage wise with xhprof off, its also about 1%.
Therefore and because this removes brittleness => RTBC.
Comment #23
Wim LeersWow, nice, 1% win!
Related: #2471216: render() should call the renderer service directly.
Comment #24
alexpottThis issue is a major task that will improve performance significantly and the disruption it introduces is limited. Per https://www.drupal.org/core/beta-changes, this is a good change to complete during the Drupal 8 beta phase. Committed 651063e and pushed to 8.0.x. Thanks!
Comment #26
joelpittetSWEET! nice work and thank you @fgm and @Fabianx!