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

Files: 
CommentFileSizeAuthor
#16 interdiff-10-15.txt15.35 KBfgm
#15 0001-Issue-2464045-Moved-twig_render_var-and-twig_drupal_.patch19.26 KBfgm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,438 pass(es). View
#13 0001-Issue-2464045-Moved-twig_render_var-and-twig_drupal_.patch18.07 KBfgm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#10 0001-Issue-2464045-Moved-twig_render_var-to-TwigExtension.patch10.11 KBfgm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,353 pass(es). View
#8 0001-Issue-2464045-Moved-twig_render_var-to-TwigExtension.patch10.1 KBfgm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,356 pass(es). View
#6 0001-Issue-2464045-Moved-twig_render_var-to-TwigExtension.patch8.73 KBfgm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#4 0001-Issue-2464045-Moved-twig_render_var-to-TwigExtension.patch5.55 KBfgm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Comments

Fabianx’s picture

Tagged for 'D8 Accelerate Dev Days' sprint

Cottser’s picture

Issue tags: +Twig
dawehner’s picture

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

Status: Active » Needs review
FileSize
5.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Maybe like this ? Informal checks seem to work.

Status: Needs review » Needs work
fgm’s picture

Status: Needs work » Needs review
FileSize
8.73 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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
fgm’s picture

Status: Needs work » Needs review
FileSize
10.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,356 pass(es). View

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

+++ 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

FileSize
10.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,353 pass(es). View

Done.

Fabianx’s picture

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

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

Status: Needs work » Needs review
FileSize
18.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Let's see if it still works.

Status: Needs review » Needs work
fgm’s picture

Status: Needs work » Needs review
FileSize
19.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,438 pass(es). View

Forgot to remove the replaced test.

fgm’s picture

FileSize
15.35 KB

Interdiff between #10 and #15.

dawehner’s picture

  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

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

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

dawehner’s picture

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

Fabianx’s picture

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

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

alexpott’s picture

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

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

Status: Fixed » Closed (fixed)

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