I finally have an understanding of how the NodeVisitor works.

As more and more in Core switches to render arrays within templates, the need for performance improvements increases again.

Fortunately I have found a great solution for a syntax that is really fast and sane at the same time:

{{ attribute(name.foo, attribute(name.foo, "key")) }}

becomes

<?php
       
if (isset($context["name"])) { $_name_ = $context["name"]; } else { $_name_ = null; }
        echo
twig_render($context, array( "#twig_reference" => TRUE, "template" => $this, "path" => array(array("name", "name"), array( "attribute", "foo"), array( "attribute", $this->getAttribute($this->getAttribute($_name_, "foo"), "key")))));
?>

As twig_render already needs to check for instanceof TwigReference it can as well also check an arbitrary array key, but instead of getting the reference directly the context is unwrapped by following the stored path.

We also know that render arrays are likely to occur, such we check for array first and only in the case that it is no array, the normal getAttribute function is called.

Note how the code makes proper use of the property that we don't need attribute keys to return references. These use the normal Twig getAttribute function.

Note: It is important to keep getAttribute as much as possible as those are replaced by fast calls to the C extension - if it is installed.

All the new magic is at compile time and while there is still work needed to make it into a proper patch and benchmark it, I'll post some draft (non-patch) here soon.

@see http://twig.sensiolabs.org/doc/functions/attribute.html

Files: 
CommentFileSizeAuthor
#28 1922304-28.patch11.3 KBCottser
FAILED: [[SimpleTest]]: [MySQL] 55,911 pass(es), 44 fail(s), and 0 exception(s).
[ View ]
#28 interdiff.txt1.6 KBCottser
#26 1922304-26.patch11.32 KBCottser
FAILED: [[SimpleTest]]: [MySQL] 55,585 pass(es), 231 fail(s), and 2 exception(s).
[ View ]
#26 interdiff.txt679 bytesCottser
#18 issue-1922304-18.diff11.32 KBFabianx
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#18 test-twig-reference-node-visitor.html_.twig_.txt223 bytesFabianx
#18 test-twig-reference-node-visitor.php_.txt1.73 KBFabianx
#16 1922304-16-twig-reference-node-visitors.patch15.87 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#16 interdiff.txt8.66 KBjoelpittet
#15 1922304-15-twig-reference-node-visitors.patch15.74 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#15 interdiff.txt7.87 KBjoelpittet
#13 test-twig-reference-node-visitor.html_.twig_.txt214 bytesmhagedon
#12 test-twig-reference-node-visitor.php_.txt1.75 KBmhagedon
#11 1922304-trying-to-fix-node-visitor-priority--11.patch7.74 KBmhagedon
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1922304-trying-to-fix-node-visitor-priority--11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#9 test-twig-reference-node-visitor.txt9.06 KBmhagedon
#8 1922304-implement-twig-reference-node-visitor--8.patch9.49 KBmhagedon
FAILED: [[SimpleTest]]: [MySQL] 53,404 pass(es), 1,041 fail(s), and 114 exception(s).
[ View ]
#7 fast-reference.clean_.txt8.35 KBmhagedon
#1 fast-reference.clean_.txt9.65 KBFabianx

Comments

Fabianx’s picture

StatusFileSize
new9.65 KB

Attached a first working implementation just with Twig.

Fabianx’s picture

Issue tags:+Performance

Forgot the performance tag.

podarok’s picture

Issue tags:+Needs Documentation

possibly this needs more documentation - looking at code and its not clear to understand

Fabianx’s picture

Bumping, this is an important issue for Twig speed.

joelpittet’s picture

The twig syntax is not very clear on what it is doing. Maybe you could provide a more concrete example of this? Is {{ attribute(,) }} in reference to an Attribute() instance or is that just a name collision?

Fabianx’s picture

Yes, I will add additional information soon on how this is working.

mhagedon’s picture

StatusFileSize
new8.35 KB

Cleaned this up with @fabianx

mhagedon’s picture

Status:Active» Needs review
StatusFileSize
new9.49 KB
FAILED: [[SimpleTest]]: [MySQL] 53,404 pass(es), 1,041 fail(s), and 114 exception(s).
[ View ]

Moved all classes into Drupal, made a proper patch, and giving testbot a chance but will probably fail. (with @fabianx)

mhagedon’s picture

StatusFileSize
new9.06 KB

Updated the test script to work with the Drupal functions. Can be run with drush scr.

Status:Needs review» Needs work

The last submitted patch, 1922304-implement-twig-reference-node-visitor--8.patch, failed testing.

mhagedon’s picture

Status:Needs work» Needs review
StatusFileSize
new7.74 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1922304-trying-to-fix-node-visitor-priority--11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Worked on this some more by myself and with @Fabianx. This patch isn't working, but uploading anyway for reference per @Fabianx. The issue we were trying is that if we make TwigReferenceNodeVisitor run after TwigNodeVisitor, it doesn't work. But it needs to run after.

mhagedon’s picture

StatusFileSize
new1.75 KB

Here's the test script.

mhagedon’s picture

Had some problems with the uploader... but this is the template referenced in the test script.

Status:Needs review» Needs work

The last submitted patch, 1922304-trying-to-fix-node-visitor-priority--11.patch, failed testing.

joelpittet’s picture

StatusFileSize
new7.87 KB
new15.74 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

@mhagedon great work on this, #11 seemed to be an interdiff instead of the diff and that's why it didn't apply also a few whitespace errors too. I am just putting this back up as your #11 patch with an interdiff from #8

joelpittet’s picture

Status:Needs work» Needs review
StatusFileSize
new8.66 KB
new15.87 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

just some spacing cleanup because I forgot to click Needs Review to activate the testbot... so.

Status:Needs review» Needs work

The last submitted patch, 1922304-16-twig-reference-node-visitors.patch, failed testing.

Fabianx’s picture

Status:Needs work» Needs review
StatusFileSize
new1.73 KB
new223 bytes
new11.32 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Okay, I cleaned this up from scratch, so no interdiff for this:

* This is actually working and almost ready for prime time.
* Next steps are to write more tests (once it passes test bot) then do some initial profiling.
* Code style and remove commented out code
* Enjoy!

I also attached the new test script and test htm.twig, which can be still run via:

$ drush scr test-twig-reference-node-visitor.php

It is exciting to see this work!

EDIT: I simplytest.me'ed it and it hide was working fine for node.html.twig in bartik!

Cottser’s picture

Here's some profiling data, for a node page with 50 threaded comments. Quite a nice improvement!

=== 8.x..8.x compared (51a944519b58c..51a9457328375):

ct  : 232,656|232,656|0|0.0%
wt  : 983,597|984,457|860|0.1%
cpu : 955,306|956,184|878|0.1%
mu  : 33,033,712|33,033,712|0|0.0%
pmu : 33,560,472|33,560,472|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a944519b58c&...

=== 8.x..nodevisitors-1922304-18 compared (51a944519b58c..51a9459592df6):

ct  : 232,656|230,904|-1,752|-0.8%
wt  : 983,597|973,023|-10,574|-1.1%
cpu : 955,306|944,392|-10,914|-1.1%
mu  : 33,033,712|32,929,480|-104,232|-0.3%
pmu : 33,560,472|33,433,472|-127,000|-0.4%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a944519b58c&...

Status:Needs review» Needs work
Issue tags:-Performance, -Needs tests, -Needs Documentation, -Needs profiling, -Twig

The last submitted patch, issue-1922304-18.diff, failed testing.

Fabianx’s picture

Status:Needs work» Needs review

#18: issue-1922304-18.diff queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Performance, +Needs tests, +Needs Documentation, +Needs profiling, +Twig

The last submitted patch, issue-1922304-18.diff, failed testing.

Cottser’s picture

I think this does need more work, I ran the theme tests locally and saw a bunch of errors like:

Argument 1 passed to Drupal\Core\Template\TwigNodeExpressionGetAttrReference::compile() must be an instance of Drupal\Core\Template\Twig_Compiler, instance of Twig_Compiler given, called in core/vendor/twig/twig/lib/Twig/Compiler.php on line 97 and defined

For example run 'Entity filtering theme test' under the Theme group.

Fabianx’s picture

ah, nice catch :)

Cottser’s picture

Assigned:Fabianx» Cottser

Talked to Fabian, I will see if I can push this along a bit further over the next few days or so.

Cottser’s picture

Status:Needs work» Needs review
StatusFileSize
new679 bytes
new11.32 KB
FAILED: [[SimpleTest]]: [MySQL] 55,585 pass(es), 231 fail(s), and 2 exception(s).
[ View ]

This should help, all tests in the 'Theme' group pass now. If testbot likes it I will work on docs and coding standards.

Status:Needs review» Needs work

The last submitted patch, 1922304-26.patch, failed testing.

Cottser’s picture

Status:Needs work» Needs review
StatusFileSize
new1.6 KB
new11.3 KB
FAILED: [[SimpleTest]]: [MySQL] 55,911 pass(es), 44 fail(s), and 0 exception(s).
[ View ]

Uncommenting the commented out code seems to make tests pass so putting this up to see what happens.

Status:Needs review» Needs work

The last submitted patch, 1922304-28.patch, failed testing.

Cottser’s picture

Assigned:Cottser» Unassigned

I haven't been able to make any progress on this. The patch in #28 has less fails from testbot but overall the patch in #26 works better. The main issue I'm seeing with #28 is that it's rendering children it shouldn't, which leads me to think that there are elements that should be references that aren't being turned into references.

Fabianx’s picture

Assigned:Unassigned» Fabianx

Thanks for bringing this along so far.

Cottser’s picture

Assigned:Fabianx» Cottser

Going to do some coding standards and other cleanup and see if I can start to understand things a bit more :)

Cottser’s picture

Assigned:Cottser» Unassigned

Unassigning for now, unfortunately I didn't have as much time in the past few days to look at this.

Fabianx’s picture

Assigned:Unassigned» Fabianx

DrupalCon!

Fabianx’s picture

Issue summary:View changes

Updated issue summary.

Cottser’s picture

Assigned:Fabianx» Unassigned
Status:Needs work» Closed (duplicate)