Found while profiling #1843746-46: Convert views/templates/views-view-field.tpl.php to Twig - this method gets called quite a bit when the same template is used multiple times in a request.

Patch coming after I do some initial profiling. This could potentially be submitted upstream as well.

CommentFileSizeAuthor
#6 1979290-6.patch1.59 KBsoulston
#1 1979290-1.patch1.62 KBstar-szr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

Status: Active » Needs review
FileSize
1.62 KB

Something like this?

Tested on a page that renders a node as well as 200 fields in a Views table via views-view-field.html.twig.

=== baseline-views-field-twig..views-field-twig-static-cache compared (517896dc57c6e..5178989df1d0f):

ct  : 116,895|115,892|-1,003|-0.9%
wt  : 478,367|474,349|-4,018|-0.8%
cpu : 449,530|444,639|-4,891|-1.1%
mu  : 17,447,208|17,453,200|5,992|0.0%
pmu : 17,658,176|17,664,136|5,960|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=517896dc57c6e&...

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

I like this!

Pretty straightforward code, has benchmarks to back it up, theme system things are called like 1000 times, so effect will be even higher (more like 25-30 ms).

6 kB more vs 4-5 ms saved is a really pretty deal!

Comments are nice, tests still pass, can't think of anything else.

It is free performance, lets take it :-).

=> RTBC

star-szr’s picture

Issue tags: +Quick fix

Thanks for reviewing @Fabianx! I think this qualifies as a quick fix :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Template/TwigEnvironment.phpundefined
@@ -110,4 +119,20 @@ protected function storage() {
+  /**
+   * Implements Twig_Environment::getTemplateClass().
+   *
+   * We override this method to add caching because it gets called multiple
+   * times when the same template is used more than once. For example, a page
+   * rendering 50 nodes without any node template overrides will use the same
+   * node.html.twig for the output of each node and the same compiled class.
+   */
+  public function getTemplateClass($name, $index = null) {

Needs to document the function params... and according the docs should be Overrides and not Implements...

soulston’s picture

Assigned: Unassigned » soulston
soulston’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: +LONDON_2013_APRIL
FileSize
1.59 KB

Re-rolled, thanks for the help @alexpott

This function is overriding so inherit the documentation using {@inheritdoc}.

catch’s picture

Is there an upstream issue/pr for this?

star-szr’s picture

I'll create a pull request later today (in about 4h).

star-szr’s picture

catch’s picture

#6: 1979290-6.patch queued for re-testing.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed dced5e1 and pushed to 8.x. Thanks!

Obviously if the Twig PR lands we can rolls this back... nice work btw!

Status: Fixed » Closed (fixed)

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

fabpot’s picture

Just for your information, the patch has been just merged upstream.

star-szr’s picture

Assigned: soulston » Unassigned

Thanks @fabpot!

star-szr’s picture