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

CommentFileSizeAuthor
#16 interdiff-10-15.txt15.35 KBfgm
#15 0001-Issue-2464045-Moved-twig_render_var-and-twig_drupal_.patch19.26 KBfgm
#13 0001-Issue-2464045-Moved-twig_render_var-and-twig_drupal_.patch18.07 KBfgm
#10 0001-Issue-2464045-Moved-twig_render_var-to-TwigExtension.patch10.11 KBfgm
#8 0001-Issue-2464045-Moved-twig_render_var-to-TwigExtension.patch10.1 KBfgm
#6 0001-Issue-2464045-Moved-twig_render_var-to-TwigExtension.patch8.73 KBfgm
#4 0001-Issue-2464045-Moved-twig_render_var-to-TwigExtension.patch5.55 KBfgm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented

Tagged for 'D8 Accelerate Dev Days' sprint

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue tags: +Twig
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue tags: +needs profiling

This sounds like a good idea but even if we do that, we should somehow know how much we gain.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
Status: Active » Needs review
FileSize
0001-Issue-2464045-Moved-twig_render_var-to-TwigExtension.patch5.55 KB

Maybe like this ? Informal checks seem to work.

Status: Needs review » Needs work

The last submitted patch, 4: 0001-Issue-2464045-Moved-twig_render_var-to-TwigExtension.patch, failed testing.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
Status: Needs work » Needs review
FileSize
0001-Issue-2464045-Moved-twig_render_var-to-TwigExtension.patch8.73 KB

The first two errors are because the calls to drupal_render_var() need to be replaced in TwigEngineTest::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'] in TwigExtension::getFilters(). This reroll should have one less fail.

Status: Needs review » Needs work

The last submitted patch, 6: 0001-Issue-2464045-Moved-twig_render_var-to-TwigExtension.patch, failed testing.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
Status: Needs work » Needs review
FileSize
0001-Issue-2464045-Moved-twig_render_var-to-TwigExtension.patch10.1 KB

Since 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.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented 
+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -58,6 +58,70 @@ public function __construct(RendererInterface $renderer) {
+    show($arg);

This should be inlined as well.

--

Thanks for the work! Looks great already!

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
FileSize
0001-Issue-2464045-Moved-twig_render_var-to-TwigExtension.patch10.11 KB

Done.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Acquia commented

Thanks 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.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Acquia commented
Status: Needs review » Needs work

On 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.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
Status: Needs work » Needs review
FileSize
0001-Issue-2464045-Moved-twig_render_var-and-twig_drupal_.patch18.07 KB

Let's see if it still works.

Status: Needs review » Needs work

The last submitted patch, 13: 0001-Issue-2464045-Moved-twig_render_var-and-twig_drupal_.patch, failed testing.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
Status: Needs work » Needs review
FileSize
0001-Issue-2464045-Moved-twig_render_var-and-twig_drupal_.patch19.26 KB

Forgot to remove the replaced test.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
FileSize
interdiff-10-15.txt15.35 KB

Interdiff between #10 and #15.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -296,4 +297,148 @@ public function attachLibrary($library) {
+   * Overrides twig_escape_filter().
+   *
+   * Replacement function for Twig's escape filter.
+   *
+   * @param \Twig_Environment $env

    It would be nice to explain why we replace it and what is the difference. 

  2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -296,4 +297,148 @@ public function attachLibrary($library) {
+      else {
+        throw new \Exception(t('Object of type "@class" cannot be printed.', array('@class' => get_class($arg))));
+      }

    Is that copied from the parent or could be throw an invalid argument exception? 

  3. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -296,4 +297,148 @@ public function attachLibrary($library) {
+    // Check for a numeric zero int or float.
+    if ($arg === 0 || $arg === 0.0) {
+      return 0;
+    }
+

    Seems a bit of an optimisation for an uncommon case, how often do we print out 0 or 0.0

Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Acquia commented

RTBC, 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.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Acquia commented

#17: This is unfortunately all out of scope here, this is just moving code around.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Sorry for my comment, didn't thought about too much.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Acquia commented
Title: Move twig_render_var to TwigExtension, inject the renderer in Twig extension and inline render() function instead of calling it » Move twig_render_var/twig_drupal_escape_filter to TwigExtension, inject the renderer in Twig extension and inline render() / show() function instead of calling it

Quick IS update

Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Acquia commented
Status: Needs review » Reviewed & tested by the community

Here is the report:

### FINAL REPORT

=== 8.0.x..8.0.x compared (552e2f110094a..552e5116e3699):

ct  : 637,784|637,784|0|0.0%
wt  : 882,098|882,914|816|0.1%
mu  : 37,264,232|37,164,704|-99,528|-0.3%
pmu : 39,639,696|39,536,048|-103,648|-0.3%

=== 8.0.x..issue-2464045 compared (552e2f110094a..552e546f49ceb):

ct  : 637,784|634,948|-2,836|-0.4%
wt  : 882,098|873,647|-8,451|-1.0%
mu  : 37,264,232|37,187,296|-76,936|-0.2%
pmu : 39,639,696|39,561,872|-77,824|-0.2%

=== SUM: 8_0_x-summary..issue-2464045-summary compared (552e55deb38d4..552e562b95405):

ct  : 637,788,929|634,952,929|-2,836,000|-0.4%
wt  : 896,997,235|890,596,507|-6,400,728|-0.7%
mu  : 37,165,409,440|37,188,002,544|22,593,104|0.1%
pmu : 39,537,254,432|39,562,589,608|25,335,176|0.1%

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

### XHPROF-LIB REPORT

+-------------------+------------+------------+------------+------------+------------+
| namespace         |        min |        max |       mean |     median |       95th |
+-------------------+------------+------------+------------+------------+------------+
| Calls             |            |            |            |            |            |
|                   |            |            |            |            |            |
| issue-2464045     |    634,948 |    639,877 |    634,953 |    634,948 |    634,948 |
| 8_0_x             |    637,784 |    642,713 |    637,789 |    637,784 |    637,784 |
|                   |            |            |            |            |            |
| Wall time         |            |            |            |            |            |
|                   |            |            |            |            |            |
| issue-2464045     |    873,647 |    988,831 |    890,597 |    890,349 |    898,486 |
| 8_0_x             |    882,914 |    984,341 |    896,997 |    896,944 |    905,233 |
|                   |            |            |            |            |            |
| Memory usage      |            |            |            |            |            |
|                   |            |            |            |            |            |
| issue-2464045     | 37,187,296 | 37,762,280 | 37,188,003 | 37,187,296 | 37,187,296 |
| 8_0_x             | 37,164,704 | 37,640,776 | 37,165,409 | 37,164,704 | 37,165,160 |
|                   |            |            |            |            |            |
| Peak memory usage |            |            |            |            |            |
|                   |            |            |            |            |            |
| issue-2464045     | 39,561,872 | 40,141,600 | 39,562,590 | 39,561,872 | 39,561,872 |
| 8_0_x             | 39,536,048 | 40,022,200 | 39,537,254 | 39,536,048 | 39,537,872 |
|                   |            |            |            |            |            |
+-------------------+------------+------------+------------+------------+------------+

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.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2471216: render() should call the renderer service directly

Wow, nice, 1% win!

Related: #2471216: render() should call the renderer service directly.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -needs profiling

This 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!

  • alexpott committed 651063e on 8.0.x 
    Issue #2464045 by fgm: Move twig_render_var/twig_drupal_escape_filter to...
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

SWEET! nice work and thank you @fgm and @Fabianx!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.