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.

CommentFileSizeAuthor
#19 core-twig-declare-render-variables-1815250-19.patch5.02 KBjday
#11 core-twig-Declare-render-variables-1815250.patch4.52 KBSalah Messaoud
#7 1815250-core-twig-Declare-render-variables.patch5.34 KBjoelpittet
#4 core-twig-Declare-render-variables-1815250-4.diff5.72 KBFabianx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented
Issue tags: +Twig

Added tag.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Issue tags: +Performance

Tagging with performance.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented
Assigned: Unassigned » Fabianx

This is up next.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented
FileSize
core-twig-Declare-render-variables-1815250-4.diff5.72 KB

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

Fabianx CreditAttribution: Fabianx commented
Status: Active » Needs review
Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented
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

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
FileSize
1815250-core-twig-Declare-render-variables.patch5.34 KB

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

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Issue summary: View changes

Add more use-cases, define syntax

areke’s picture

areke CreditAttribution: areke commented
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

podarok
🇺🇦 he/him/his
Primary language Ukrainian
Location Ukraine
CreditAttribution: podarok commented
Status: Needs work » Needs review

7: 1815250-core-twig-Declare-render-variables.patch queued for re-testing.

Status: Needs review » Needs work

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

Salah Messaoud’s picture

Salah Messaoud CreditAttribution: Salah Messaoud commented
Assigned: Fabianx » Unassigned
Status: Needs work » Needs review
FileSize
core-twig-Declare-render-variables-1815250.patch4.52 KB

Status: Needs review » Needs work

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

balintcsaba’s picture

balintcsaba CreditAttribution: balintcsaba commented
Assigned: Unassigned » balintcsaba
balintcsaba’s picture

balintcsaba CreditAttribution: balintcsaba commented
Assigned: balintcsaba » Unassigned
saltednut’s picture

saltednut
she/her
Primary language English
Location Chicago
CreditAttribution: saltednut commented
Status: Needs work » Needs review

11: core-twig-Declare-render-variables-1815250.patch queued for re-testing.

Status: Needs review » Needs work

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

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

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

jday CreditAttribution: jday commented

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

jday’s picture

jday CreditAttribution: jday commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
core-twig-declare-render-variables-1815250-19.patch5.02 KB

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

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
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.