Problem/Motivation

Visiting node/1 with the standard install, anonymous, no page cache, warm caches, takes 39,000 function calls.

Equivalent Drupal 7 requests are between 5-10,000 function calls.

One of the worst offenders is Drupal::service() - this is called 397 times, and there are stack calls underneath it, so it's responsible for thousands of function calls.

One of the biggest callers of Drupal::service() is t()

One of the biggest callers of t() is system_region_list()

system_region_list() is called three times during the request. It has no static cache. It ends up translation the region names 60 times. 60 calls to t() = ~542 function calls.

We never print the region names except in block admin.

Proposed resolution

Don't translate the fucking human readable region names when we only want the machine names anyway.

Don't know exactly how much this saves, but it's probably 2-3ms per request. However this is for every single request assuming a site has block module enabled.

There's an extra hidden bonus. Once this is removed, we only have 21 calls to t() on this page. If we remove those, we'd also save initializing the translation service, and fetching the locale cache (when not viewing the site in English) - this is very feasible to get to I think.

Remaining tasks

TBD.

User interface changes

None.

API changes

Extra param to system_region_list()

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Don't know exactly how much this saves, but it's probably 2-3ms per request.

That seems a bit much?

There's an extra hidden bonus. Once this is removed, we only have 21 calls to t() on this page. If we remove those, we'd also save initializing the translation service, and fetching the locale cache (when not viewing the site in English) - this is very feasible to get to I think.

… because all translated strings are render cached? Or? (It sounds like you're saying we can literally have zero t() calls!)


+++ b/core/modules/block/src/BlockRepository.php
@@ -57,6 +57,16 @@ protected function getRegionNames() {
+   * @return array

Nit: string[].

catch’s picture

I think it's 2-3ms with xhprof profiling overhead.

… because all translated strings are render cached? Or? (It sounds like you're saying we can literally have zero t() calls!)

Right, we should only translate strings when rendering. So the only calls to t() should be 1. cold render cache 2. silly cases like this.

Uploading xhprof screenshot of the remaining calls to t() - as you can see most of them fall into category #2.

catch’s picture

There are several other calls to the actual translation services - but again most of these aren't for strings that will actually get rendered either.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Quickfix

There are several other calls to the actual translation services - but again most of these aren't for strings that will actually get rendered either.

Looking forward to patches for those :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, system_region_list.patch, failed testing.

Wim Leers’s picture

PHP Fatal error:  Call to undefined function Drupal\block\system_region_list() in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/block/src/BlockRepository.php on line 66

Unit test failure.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
4.78 KB
2.03 KB

WFM, but I see no reason to change the name of the method, it's still giving us back names (just machine names).

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Looking at this maybe the active theme should also have the regions injected?

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
6.7 KB

Something like this... feel free to ignore - but I think it makes sense for the active theme to know about it's regions. No interdiff because it's a different approach.

neclimdul’s picture

I kinda of like alex's approach. What I don't like is its only relevant to the active theme. I looked at this earlier and there are like 10 calls to system_region_list() and only 3 are looking for the labels. It seems like those other calls can't use this because of the active theme limitation. :(

catch’s picture

Status: Needs review » Reviewed & tested by the community

I like #10, moving back to RTBC for that (which I think I can do since it's completely different from my patch). I was a bit shocked when I found this issue that we still had system_region_list() at all, so moving away from it slowly is nice.

Fabianx’s picture

RTBC + 1, the approach in #10 is great, we can still follow-up with more things.

tim.plunkett’s picture

+++ b/core/modules/block/tests/src/Unit/BlockRepositoryTest.php
@@ -62,17 +70,7 @@ protected function setUp() {
-    $this->blockRepository = $this->getMockBuilder('Drupal\block\BlockRepository')
...
+    $this->blockRepository = new BlockRepository($entity_manager, $theme_manager, $this->contextHandler);

Always feels good to do this. +1

catch’s picture

I crossposted with neclimdul. It would be good to get rid of system_region_list() altogether (or at least to have a 100% replacement for the cases where labels aren't needed), but I think this is a good interim step to fix this issue too. Leaving RTBC a bit longer.

  • catch committed cf4f003 on 8.0.x
    Issue #2497259 by catch, tim.plunkett, alexpott: system_region_list()...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

David_Rothstein’s picture

Version: 8.0.x-dev » 7.x-dev
Priority: Major » Normal
Status: Fixed » Patch (to be ported)
Issue tags: +Needs backport to D7

One of the biggest callers of t() is system_region_list()

system_region_list() is called three times during the request. It has no static cache. It ends up translation the region names 60 times.

Did a quick test with Drupal 7 and the same thing applies there - 60 t() calls within system_region_list() on the front page of a standard install. Not sure if it's a significant performance impact, but seems pretty useless either way.

I am guessing the earlier patch in this issue (#7) is the one to look at for backport.

Could have sworn there was already an issue similar to this somewhere actually.... but maybe not.

David_Rothstein’s picture

Could have sworn there was already an issue similar to this somewhere actually.... but maybe not.

Found #1759306: Many unecessary calls to t() for strings that are never shown anywhere ... but it's broader than this one, so no overlap really.

  • catch committed cf4f003 on 8.1.x
    Issue #2497259 by catch, tim.plunkett, alexpott: system_region_list()...

  • catch committed cf4f003 on 8.3.x
    Issue #2497259 by catch, tim.plunkett, alexpott: system_region_list()...

  • catch committed cf4f003 on 8.3.x
    Issue #2497259 by catch, tim.plunkett, alexpott: system_region_list()...
Fabianx’s picture

Issue tags: +Drupal bugfix target
aerozeppelin’s picture

Status: Patch (to be ported) » Needs review
FileSize
4.76 KB

Patch for D7. Borrowed from #7.

Status: Needs review » Needs work

The last submitted patch, 24: 2497259-24.patch, failed testing.

aerozeppelin’s picture

Status: Needs work » Needs review
FileSize
538 bytes
4.77 KB

Fix for failing a test in #24.

Fabianx’s picture

Thanks for your work on this!

Looks already really good and keeps BC nicely.

A code comment though:

+++ b/modules/system/system.module
@@ -2716,13 +2716,17 @@ function system_region_list($theme_key, $show = REGIONS_ALL) {
-  // If requested, suppress hidden regions. See block_admin_display_form().
-  foreach ($info['regions'] as $name => $label) {
-    if ($show == REGIONS_ALL || !isset($info['regions_hidden']) || !in_array($name, $info['regions_hidden'])) {
-      $list[$name] = t($label);
+    // If requested, suppress hidden regions. See block_admin_display_form().
+    foreach ($info['regions'] as $name => $label) {
+      if ($show == REGIONS_ALL || !isset($info['regions_hidden']) || !in_array($name, $info['regions_hidden'])) {
+        if ($labels) {
+          $list[$name] = t($label);
+        }
+        else {
+          $list[$name] = $name;
+        }
+      }
     }
-  }
-

nit - Why the indentation change here?

aerozeppelin’s picture

Your welcome. Thank you for reviewing my patch. :D
Oops! I overlooked the change in indentation. Here is the updated patch.

Fabianx’s picture

Status: Needs review » Needs work

Thank you for a new patch. It was a pleasure reviewing it again in more detail:

It will need some more work unfortunately as I found some things:

  1. +++ b/modules/block/block.module
    @@ -265,7 +265,7 @@ function block_page_build(&$page) {
    -    foreach (array_keys($all_regions) as $region) {
    +    foreach (system_region_list($theme, REGIONS_ALL, FALSE) as $region) {
    

    I don't think this change is useful as we already have called the function and need the labels.

    Lets leave the old code here.

  2. +++ b/modules/system/system.module
    @@ -2708,7 +2708,7 @@ function system_find_base_themes($themes, $key, $used_keys = array()) {
      * @return
      *   An array of regions in the form $region['name'] = 'description'.
      */
    -function system_region_list($theme_key, $show = REGIONS_ALL) {
    +function system_region_list($theme_key, $show = REGIONS_ALL, $labels = TRUE) {
    

    We will need to update the doxygen as well as now the return value depends on the new parameter.

  3. +++ b/modules/system/system.module
    @@ -2747,7 +2751,7 @@ function system_system_info_alter(&$info, $file, $type) {
     function system_default_region($theme) {
    -  $regions = array_keys(system_region_list($theme, REGIONS_VISIBLE));
    +  $regions = array_keys(system_region_list($theme, REGIONS_VISIBLE, FALSE));
       return isset($regions[0]) ? $regions[0] : '';
    

    The array_keys() must be removed.

aerozeppelin’s picture

Status: Needs work » Needs review
FileSize
2.06 KB
4.63 KB

@Fabianx, Sure thing :D Changes from #29.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Little nit, but setting to RTBC. Looks great to me now!

+++ b/modules/system/system.module
@@ -2705,10 +2705,16 @@ function system_find_base_themes($themes, $key, $used_keys = array()) {
+ *   or $region['name'] = 'name', if $labels is not set.

nit - Maybe: "if $labels is set to FALSE" ?

aerozeppelin’s picture

Changes as per #31.

joseph.olstad’s picture

awesome! thanks

joseph.olstad’s picture

Did some quick tests on the latest patch #32 , works as expected
I also did some very unsophisticated performance profiling , it seems to shave off a noticeable amount of time off of a cache clear (tested on a site that has a lot of t strings and translations)

I did not notice a difference elsewhere however my performance profiling tests were very basic to the naked eye. I'll take it just for the cache clear improvement.

have only used this patch for a few minutes, so far so good. Using it on 7.50 with several other patches. Note; my test system has an SSD drive, so its very low latency, others might notice an even greater performance improvement.

Fabianx’s picture

Assigned: Unassigned » stefan.r

#34: Thanks for testing, moving over to Stefan for review / commit.

stefan.r’s picture

Assigned: stefan.r » Unassigned
Issue tags: -Needs backport to D7 +Pending Drupal 7 commit

Looks great! Should we clarify in the system_region_list() docs that you'd want to set $labels = FALSE for performance reasons?

Nit: s/(Optional) boolean/(optional) Boolean/

Fabianx’s picture

#36: Sounds good to add to the docs a little.

stefan.r’s picture

Status: Reviewed & tested by the community » Needs review

back to NR for #36

stefan.r’s picture

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, this continues to be marked as ready for commit.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 39: 2497259-39.patch, failed testing.

Fabianx’s picture

Status: Needs work » Reviewed & tested by the community

Race condition

  • stefan.r committed 809978d on 7.x
    Issue #2497259 by aerozeppelin, catch, tim.plunkett, alexpott:...
stefan.r’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit +Dublin2016

Re-reviewed with Fabianx, and committed and pushed to 7.x, thanks!

David_Rothstein’s picture

Status: Fixed » Needs review
FileSize
498 bytes
 function system_default_region($theme) {
-  $regions = array_keys(system_region_list($theme, REGIONS_VISIBLE));
-  return isset($regions[0]) ? $regions[0] : '';
+  $regions = system_region_list($theme, REGIONS_VISIBLE, FALSE);
+  $region_0 = current($regions);
+  return isset($region_0) ? $region_0 : '';
 }

This changes the behavior of the function (albeit in a pretty minor way) since current() returns FALSE on an empty array, not NULL.

Here's a quick followup for that.

David_Rothstein’s picture

Issue tags: +7.51 release notes

Also adding this to CHANGELOG.txt, since it's an API addition with a change record.

stefan.r’s picture

Status: Needs review » Reviewed & tested by the community

We've manually tested this and this looks to be fine. I don't see how this would go wrong, so RTBC

  • stefan.r committed 18e46dc on 7.x
    Issue #2497259 by David_Rothstein: system_region_list() unnecessarily...
stefan.r’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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