Problem/Motivation

You need to check at least once per request whether the active theme you use is enabled.

Currently this is done by loading all themes.

Proposed resolution

Just load one of them.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#17 histogram_interleaved.png9.29 KBWim Leers
#17 histogram_facet.png9.61 KBWim Leers
#11 interdiff.txt818 bytesclaudiu.cristea
#10 2538970-10.patch4.09 KBclaudiu.cristea
#10 2538970-10.patch4.09 KBclaudiu.cristea
#7 interdiff.txt645 bytesclaudiu.cristea
#6 improve_the_speed_of-2538970-6.patch3.85 KBclaudiu.cristea
#3 2538982-1.patch3.22 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

Note, this improves not nothing, but it doesn't change the overall time yet, because #2531958: Try to do less in theme_get_settings() on every request also lists all of it.

Function Name Calls Diff Calls
Diff%		 Incl. Wall
Diff
(microsec)		 IWall
Diff%		 Incl.
MemUse
Diff
(bytes)		 IMemUse
Diff%		 Incl.
PeakMemUse
Diff
(bytes)		 IPeakMemUse
Diff%
Current Function
Drupal\Core\Theme\ThemeAccessCheck::checkAccess 0 0.0% -95 -16.8% -61,816 -108.7% 72 0.1%
Exclusive Metrics Diff for Current Function 6 6.3% -44,400 -71.8% 0 0.0%
Parent function
Drupal\Core\Theme\ThemeNegotiator::determineActiveTheme 0 N/A% -95 -100.0% -61,816 -100.0% 72 100.0%
Child functions
Drupal\Core\Extension\ThemeHandler::listInfo -1 -100.0% -243 -255.8% -76,856 -124.3% -51,896 -72077.8%
Drupal\Core\Theme\ThemeInitialization::getActiveThemeByName 1 100.0% 142 149.5% 58,752 95.0% 51,968 72177.8%
Drupal\Core\Theme\ActiveTheme::getStatus 1 100.0% 0 0.0% 688 1.1% 0 0.0%
Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented

Is there a patch to review?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Active » Needs review
FileSize
2538982-1.patch3.22 KB

Ehm sure here it is. Ignore the NID of the patch :)

Status: Needs review » Needs work

The last submitted patch, 3: 2538982-1.patch, failed testing.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented

Looks great, just need to fix the test failures.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Status: Needs work » Needs review
FileSize
improve_the_speed_of-2538970-6.patch3.85 KB

The problem with the tests is because of the architecture of ThemeInitialization::getActiveThemeByName(). When ThemeInitialization::getActiveThemeByName() tries to return a theme that is not installed he calls ThemeInitialization::getActiveTheme() with a fake theme called core. From my point of view this a bad design but this is not subject of this issue.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
FileSize
interdiff.txt645 bytes

Forgot the interdiff.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

Good observation ... I'll dig into that later, but yeah we should fix the design there properly.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

The problem with the tests is because of the architecture of ThemeInitialization::getActiveThemeByName(). When ThemeInitialization::getActiveThemeByName() tries to return a theme that is not installed he calls ThemeInitialization::getActiveTheme() with a fake theme called core. From my point of view this a bad design but this is not subject of this issue.

Do you mind opening up an issue for that?

+++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
@@ -160,6 +160,7 @@ public function getActiveTheme(Extension $theme, array $base_themes = []) {
+    $values['status'] = $values['name'] != 'core';

So if its core its disabled? We should have a least some form of documentation

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
FileSize
2538970-10.patch4.09 KB
2538970-10.patch4.09 KB

New issue created #2545192: Don't assign 'core' to a theme that cannot be negotiated or is not in the installed theme list.

Added some docs as you suggested in #9.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
FileSize
interdiff.txt818 bytes

Ouch!

Status: Needs review » Needs work

The last submitted patch, 10: 2538970-10.patch, failed testing.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Status: Needs work » Needs review

Accidentally I added the same patch twice in #10. Back to NR.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

This looks alright for me now, but yeah I fear we need to provide some benchmarking + beta evaluation to judge whether this is useful.
Note: if you have more themes enabled, (which is the workflow if you have the themekey module, you end up in those kind of problems

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented

@dawehner, can you point me to some docs, best practices, writeups on how to provide an acceptable benchmark?

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Issue tags: +needs profiling
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: -needs profiling
FileSize
histogram_facet.png9.61 KB
histogram_interleaved.png9.29 KB

I'm afraid this is a dead end :(

EDIT: tested with 1000 requests to /contact, as the anon users, with page cache off.

Before
Requests per second:    14.11 [#/sec] (mean)
Time per request:       70.888 [ms] (mean)
Time per request:       70.888 [ms] (mean, across all concurrent requests)
Transfer rate:          195.59 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    68   71   1.2     71      76
Waiting:       60   64   1.2     63      69
Total:         68   71   1.2     71      76

Percentage of the requests served within a certain time (ms)
  50%     71
  66%     71
  75%     71
  80%     72
  90%     72
  95%     73
  98%     74
  99%     75
 100%     76 (longest request)
After
Requests per second:    13.93 [#/sec] (mean)
Time per request:       71.765 [ms] (mean)
Time per request:       71.765 [ms] (mean, across all concurrent requests)
Transfer rate:          193.20 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    67   72   1.3     71      78
Waiting:       60   64   1.3     64      71
Total:         67   72   1.3     72      78

Percentage of the requests served within a certain time (ms)
  50%     72
  66%     72
  75%     72
  80%     73
  90%     73
  95%     74
  98%     75
  99%     76
 100%     78 (longest request)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

a) to be clear #2531958: Try to do less in theme_get_settings() on every request still has a listInfo call, so this is why things are a bit slower here
b) This doesn't tell us any statistically relevant data, see standard derivation. We need to have more iterations in order to tell something properly

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

Here's what I got with the /contact as the front page. xdebug and all caching turned off. Twig debug turned off and twig caching still on.

Also I enabled all the modules and a few contrib modules too but still accessing that page anonymous so it shouldn't make I difference I'd expect.

=== 8.0.x..8.0.x compared (5659123816918..565914310b034):

ct  : 170,827|170,827|0|0.0%
wt  : 632,030|633,399|1,369|0.2%
mu  : 33,371,040|33,373,152|2,112|0.0%
pmu : 35,105,000|35,107,632|2,632|0.0%

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

=== 8.0.x..themeaccesscheck compared (5659123816918..5659177493c27):

ct  : 170,827|170,851|24|0.0%
wt  : 632,030|640,893|8,863|1.4%
mu  : 33,371,040|33,387,336|16,296|0.0%
pmu : 35,105,000|35,121,464|16,464|0.0%

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

=== SUM: 8_0_x-summary..themeaccesscheck-summary compared (565918537d922..5659186b7d6e0):

ct  : 17,091,157|17,093,710|2,553|0.0%
wt  : 69,237,937|69,699,950|462,013|0.7%
mu  : 3,337,669,336|3,339,222,864|1,553,528|0.0%
pmu : 3,511,345,104|3,512,874,752|1,529,648|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=565918537d922&... 8_0_x-summary..themeaccesscheck-summary

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Needs work

@claudiu.cristea is there something else we could test directly because page hits aren't significantly different it seems from the tests above.

Setting to NW for update to profiling scenario/instructions.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

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

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

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

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should 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.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should 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.

Version: 9.5.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. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.