Problem/Motivation

There is some generalized logic in twig_render_template that does some extra work during normal rendering to support development rendering. This is not strictly necessary though.

Since this is a hot path it makes sense to have it as efficient as possible.

Steps to reproduce

Proposed resolution

Move all of the debug logic into the debug switch so the "production" path is optimized.

Remaining tasks

Benchmarks?

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#10 3192830-10.patch1.96 KBneclimdul
#10 3192830-10.interdiff.txt680 bytesneclimdul
#8 3192830-8.patch1.96 KB_utsavsharma
#8 d10-diff.txt1.96 KB_utsavsharma
twig_render_template-micro-optimization.patch1.98 KBneclimdul

Issue fork drupal-3192830

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

neclimdul created an issue. See original summary.

neclimdul’s picture

neclimdul
He/Him
commented
Status: Active » Needs review

Admittedly, there isn't a lot of internal time in this method so this is far from a high priority optimization but some basic synthetic tests showed something like a 25% improvement in the internal time of the method. Nice! Since a bulk of the logic is in the implode call that makes sense.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs subsystem maintainer review, +Needs reroll

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Tagging for a reroll for 10.1.x
Tagging for subsystem maintainer of the theme layer to get their input

But if the logic is for development and doesn't interfere with the development functionality sounds like a good clean up.

Thanks!

_utsavsharma’s picture

_utsavsharma commented
StatusFileSize
new d10-diff.txt1.96 KB
new 3192830-8.patch1.96 KB

Reroll for 10.1.x.

neclimdul’s picture

neclimdul
He/Him
commented
Issue tags: -Needs subsystem maintainer review, -Needs reroll

The code is a bit different because of Twig 3 which broke the original patch. Probably just needs a rewrite not a reroll or maintainer review.

neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs work » Needs review
StatusFileSize
new 3192830-10.interdiff.txt680 bytes
new 3192830-10.patch1.96 KB

Not so big a change it turns out. Here's a patch that should work.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Thanks for the quick turnaround!

Did a quick test on Drupal 10.1.x on a standard install during debug on in my development.services.yml file

  http.response.debug_cacheability_headers: true
  twig.config:
    auto_reload: true
    cache: false
    debug: true

And debugging still works.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 3192830-10.patch, failed testing. View results

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

neclimdul opened merge request !8275

neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs work » Needs review

reroll in MR

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

nice clean-up!

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Not convinced this is really going to make any measurable difference in the real world, but it makes the code a bit easier to read.

Backported to 10.3.x as a cleanup with no functional changes.

Committed and pushed 0ba6b793c6 to 11.x and 1eac567fbb to 11.0.x and d7549f3c3a to 10.4.x and ffdafb02cd to 10.3.x. Thanks!

  • longwave committed ffdafb02 on 10.3.x 
    Issue #3192830 by neclimdul: twig_render_template micro optimization

(...

  • longwave committed d7549f3c on 10.4.x 
    Issue #3192830 by neclimdul: twig_render_template micro optimization

(...

  • longwave committed 1eac567f on 11.0.x 
    Issue #3192830 by neclimdul: twig_render_template micro optimization

(...

  • longwave committed 0ba6b793 on 11.x 
    Issue #3192830 by neclimdul: twig_render_template micro optimization
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

longwave closed merge request !8275

neclimdul’s picture

neclimdul
He/Him
commented

Nice!

Yeah, probably not measurable from a wall clock perspective with current PHP performance hence the title. However, it removes one assignment, one function call to implode, and the admittedly trivial string concats implode is doing from every twig_render. This means on a stock Drupal install it saves 54 function calls and assignments on an uncached request to the frontpage so its at least measurable from that metric. Since most sites are probably more complex then that and as you said its actually a bit cleaner it seemed worth while.

Status: Fixed » Closed (fixed)

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