Followup from: #1696786: Integrate Twig into core: Implementation issue

Motivation

Currently all variables that are printed go through twig_render, but for the majority of strings this is unnecessary.

  echo twig_render($this->getContext("label"));

While the checks are fast, they incur overhead, the non-render version looks like:

  if (isset($context['label']) { $_label_ = $context['label'] } else { $_label_ = null; }
  echo $_label_;

This is much closer to the original node.tpl.php.

Solution

If PHP would declare all render elements (originally it should have been only one in a function) like:

function template_get_render_variables_node() {
  return array('content', 'title_prefix', 'title_suffix');
}

or even within the $variables from the preprocess like:

  $variables['#render_variables'] = array('content', 'title_prefix', 'title_suffix');

In that case our TwigExtension could compile time optimize the injection of twig_render() calls only where it is necessary. This especially improves the performance on critical paths like theme_link or theme_field.

Alternatively we could have {% autorender on %} / {% autorender off %} blocks if the above is not possible to make things explicit in templates.

As a followup to views-view-field.html.twig conversion, what we probably also need to do is:

- Be able to type variables in $vars like:

$vars['#metadata']['object'][] = 'field';

A TwigNodeVisitor could then update the views-view-field.html.twig to look almost exactly like the theme_ function.

The proposal is to be able to specify this metadata as part of theme() hook.

In that Twig core performance can be increased much.

The POC below above can be used as reference.

Files: 
CommentFileSizeAuthor
#19 core-twig-declare-render-variables-1815250-19.patch5.02 KBjday
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 40,740 pass(es), 18,150 fail(s), and 12,153 exception(s). View
#11 core-twig-Declare-render-variables-1815250.patch4.52 KBSalah Messaoud
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core-twig-Declare-render-variables-1815250.patch. Unable to apply patch. See the log in the details link for more information. View
#7 1815250-core-twig-Declare-render-variables.patch5.34 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1815250-core-twig-Declare-render-variables.patch. Unable to apply patch. See the log in the details link for more information. View
#4 core-twig-Declare-render-variables-1815250-4.diff5.72 KBFabianx
PASSED: [[SimpleTest]]: [MySQL] 47,856 pass(es). View

Comments

Fabianx’s picture

Issue tags: +Twig

Added tag.

catch’s picture

Issue tags: +Performance

Tagging with performance.

Fabianx’s picture

Assigned: Unassigned » Fabianx

This is up next.

Fabianx’s picture

FileSize
5.72 KB
PASSED: [[SimpleTest]]: [MySQL] 47,856 pass(es). View

Here is a first stab at this.

The idea is simple:

  • Collect all render variables in $variables['theme_render_variables'] (automatically add defaults and 'render element')
  • Save this render variables for this template inside the environment
  • Optimize render() calls if this information is available for this template

There are not many templates that actually use render variables different from render element or the provided defaults. It is mainly node, comment, comment-wrapper, page and html.

That have $content or $page as renderable array. Defaults are title_prefixed and title_suffix.

Still todo:

  • * Need to make sure the template class name is based on the content of dynamic vars.
  • * Need to make sure this still works with sub templates.

These two TODOs are to be understand as follows:

* If the template name includes the dynamic vars in the hash (in this case the theme_render_variables), then an added / changed render variable always leads to a template that is compatible with that.

* Subtemplates used via include, embed, use, extends still work without problems, _but_ are not optimized.

The problem here is that an "include" or "use" might have nothing todo with the parent template, so fixing this here is a "won't fix", and I will rather use the {% autorender off %} approach for those as core does not need to use or support this.

It can be easily done for extends though as we can register the dynamic vars for the parent on behalf of the child template. It needs thought for embed, which is a hybrid of "include" and "extends".

Fabianx’s picture

Status: Active » Needs review
Fabianx’s picture

Title: Declare render variables in PHP and remove superfluous twig_render() calls » Type twig variables in PHP and remove superfluous render_var() and getAttribute() calls from the generated code
Priority: Normal » Major

Changing title and priority to reflect on the huge performance savings that typing data makes possible.

joelpittet’s picture

FileSize
5.34 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1815250-core-twig-Declare-render-variables.patch. Unable to apply patch. See the log in the details link for more information. View

Re-rolled and reviewed this patch.

It feels like the extra declaration of $variables['theme_render_variables'] array is separating the association with the renderable variables with the fact that they are that. It would be much better if we could avoid the explicit declaration or define their 'type' right on the array like:
array('#render_type' => 'renderable_array') or array('#render_type' => 'renderable'). That's just my preference from DX point of view.

Scenario

Here is what I got on 50 nodes on the front page.

=== 8.x..8.x compared (523f1dee40c1a..523f1e3aba264):

ct  : 234,611|234,611|0|0.0%
wt  : 1,095,132|1,096,124|992|0.1%
cpu : 1,029,136|1,029,316|180|0.0%
mu  : 44,263,728|44,263,728|0|0.0%
pmu : 44,619,080|44,619,080|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=523f1dee40c1a&...

=== 8.x..1815250-reduce-render_var-calls compared (523f1dee40c1a..523f1f1f50546):

ct  : 234,611|233,734|-877|-0.4%
wt  : 1,095,132|1,091,788|-3,344|-0.3%
cpu : 1,029,136|1,026,034|-3,102|-0.3%
mu  : 44,263,728|43,853,424|-410,304|-0.9%
pmu : 44,619,080|44,431,032|-188,048|-0.4%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=523f1dee40c1a&...

joelpittet’s picture

Issue summary: View changes

Add more use-cases, define syntax

areke’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll

This patch needs to be re-rolled because it doesn't apply anymore.

podarok’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 7: 1815250-core-twig-Declare-render-variables.patch, failed testing.

Salah Messaoud’s picture

Assigned: Fabianx » Unassigned
Status: Needs work » Needs review
FileSize
4.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core-twig-Declare-render-variables-1815250.patch. Unable to apply patch. See the log in the details link for more information. View

Status: Needs review » Needs work

The last submitted patch, 11: core-twig-Declare-render-variables-1815250.patch, failed testing.

balintcsaba’s picture

Assigned: Unassigned » balintcsaba
balintcsaba’s picture

Assigned: balintcsaba » Unassigned
brantwynn’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 11: core-twig-Declare-render-variables-1815250.patch, failed testing.

joelpittet’s picture

See my concerns and suggestions in #7.

I like the general idea of not calling render_var() on every printed variable, but there needs to be an in context way of doing this so you don't have to declare renderable arrays in yet another array.

One other consideration would be to make all renderable arrays into renderable objects that maybe Extend from Twig_Markup to avoid double escaping issues when we turn on autoescaping?

But the main concern is that we should likely key all the renderable arrays we are building:

  $variables['#render_variables'] = array('content', 'title_prefix', 'title_suffix');
...
  $variables['title_prefix'] = array('#markup' => 'bob');
  $variables['content'] = array('#markup' => 'loblaw');
  $variables['title_suffix'] = array('#markup' => 'law blog');

compared to:

  $variables['title_prefix'] = array('#markup' => 'bob', '#render_type' => 'renderable');
  $variables['content'] = array('#markup' => 'loblaw', '#render_type' => 'renderable');
  $variables['title_suffix'] = array('#markup' => 'law blog', '#render_type' => 'renderable');

Though now that I write this out, nested renderable elements will have an issue either way...

Maybe renderable object where it internally calls drupal_render() on it's render() method which is auto called on __toString(). #pipedream.

jday’s picture

new contributor at DrupalCon Austin, working on re-rolling last patch.

jday’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
5.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 40,740 pass(es), 18,150 fail(s), and 12,153 exception(s). View

Re-rolled patch

Status: Needs review » Needs work

The last submitted patch, 19: core-twig-declare-render-variables-1815250-19.patch, failed testing.

joelpittet’s picture

Status: Needs work » Closed (won't fix)

Closing this as won't fix because it adds some complexity to DX via way of separating what you are acting on from it's type and that won't help with later D9ish goal of renderable objects.
Also hasn't had work in nearly a year.

The performance profiling weren't that great either.