_theme_process_registry() includes any include file (could be a template or foo.admin.inc) when building the registry. It looks like including the file isn't necessary.

I traced the include back to #168028: both theme functions and templates may need include files, when that patch went in hook_theme() and especially include file/template handling was very different, and as far as I can tell it's no longer needed. Originally the include had a comment 'just to be safe', this was lost in the past three years.

This saves at least 40ms off a registry rebuild, but more importantly 5mb of memory, with just core modules enabled. Sites with lots of modules, lots of includes and lots of templates are going to see considerably bigger savings.

As well as this, all those included files, many of which are never going to be used if they're templates, will pollute opcode caches - potentially causing evictions etc. - so this should help on cold starts a fair bit.

Theme tests pass but didn't run the whole suite.

If I run drush cc all then hit the front page without apc enabled, that requires 45mb of memory with all the caches being rebuilt.

I confirmed that the total memory usage with this patch dropped to 41mb. So if these includes really aren't needed, that may end up helping with issues like #1008362: 32M is sometimes not enough memory to install Drupal 7, module page submit - anywhere else where a single request can end up building all global caches.

If this doesn't break things horribly, it may win the award for most kilobytes of memory saved per line of patch ;)


CommentFileSizeAuthor
#30 1177762-30.patch1.99 KBlongwave
#25 1177762-25-includes.patch1.29 KBlongwave
#24 1177762-24-includes.patch1.35 KBlongwave
#22 1177762-file.patch826 bytescatch
#22 1177762-includes.patch875 bytescatch
#21 1177762-includes.patch783 bytescatch
#21 1177762-file.patch675 bytescatch
#6 theme_registry_rebuild-1177762-6.patch605 bytesjoelpittet
fiiiiiiiles.png178.25 KBcatch
files.patch502 bytescatch

Issue fork drupal-1177762

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

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

This may not be possible due to process functions - at least not if we're relying on function_exists() to see whether they exist or not in include files, see what the bot says though.

Status: Needs review » Needs work

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

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs work » Closed (works as designed)

Shame.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Closed (works as designed) » Active

This patch won't work, but we could find out if the functions exist another way, such as using the registry regexp.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Issue summary: View changes
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Active » Needs review
StatusFileSize
new theme_registry_rebuild-1177762-6.patch605 bytes

@catch this may still valid in 8.1.x but not 8.0.x. Maybe you have another way of dealing but I'm reposting the patch for our current 8.0.x

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Version: 8.0.x-dev » 8.1.x-dev
Status: Needs review » Active

Bumping

Status: Active » Needs work

The last submitted patch, 6: theme_registry_rebuild-1177762-6.patch, failed testing.

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

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now 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.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now 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.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now 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.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now 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.

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

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now 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.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now 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.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.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.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). 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.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now 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: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

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.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Title: Theme registry rebuild needlessly loads dozens of include files » Remove 'includes' and possibly 'file' support from hook_theme()
Category: Bug report » Task
Issue tags: +Bug Smash Initiative

This code still exists, but 'includes' isn't documented in hook_theme() at all: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21...

And I'm not convinced we need to support 'file' any more now that actual theme implementations are in templates.

Soo.. maybe we can deprecate both keys for removal in 10.x? Or at least 'includes' if file is still used somehow.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Needs work » Needs review
StatusFileSize
new 1177762-file.patch675 bytes
new 1177762-includes.patch783 bytes

See if any of this is dead code first..

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
StatusFileSize
new 1177762-includes.patch875 bytes
new 1177762-file.patch826 bytes
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

3k failed tests looks promising!

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
StatusFileSize
new 1177762-24-includes.patch1.35 KB

I don't think we can get rid of 'file' easily as there are plenty of usages in core modules but there only seems to be one use of 'includes' that we can refactor away.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
StatusFileSize
new 1177762-25-includes.patch1.29 KB
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

Turns out Views uses it as well, but we might be able to remove that too.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

OK to do 'files' we'll probably need #1804614: [meta] Consolidate similar twig templates and properly use theme suggestions in core and various related issues done first, but good spot on the one remaining usage of 'includes', looks like we just need to move the preprocess out of that file, and possibly a follow-up to refactor the remaining function in it.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Title: Remove 'includes' and possibly 'file' support from hook_theme() » Remove 'includes' support from hook_theme()
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Needs work

1291 failures, I think this is all because of

      // For the views module we ensure views.theme.inc is included.
      if ($def['provider'] == 'views') {
        if (!isset($hooks[$def['theme']]['includes'])) {
          $hooks[$def['theme']]['includes'] = [];
        }
        if (!in_array('views.theme.inc', $hooks[$def['theme']]['includes'])) {
          $hooks[$def['theme']]['includes'][] = $module_dir . '/views.theme.inc';
        }
      }
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Needs review
StatusFileSize
new 1177762-30.patch1.99 KB

Nope, there is a bigger issue: 'file' sets up 'includes' for later cached calls:

        if (isset($info['file'])) {
          $include_file = isset($info['path']) ? $info['path'] : $path;
          $include_file .= '/' . $info['file'];
          include_once $this->root . '/' . $include_file;
          $result[$hook]['includes'][] = $include_file;
        }

We can still deprecate 'includes' from being declared in the first place though.

Status: Needs review » Needs work

The last submitted patch, 30: 1177762-30.patch, failed testing. View results

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.

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.

berdir’s picture

berdir
Primary language German
Location Switzerland
CreditAttribution: berdir at MD Systems GmbH commented
Status: Needs work » Postponed

#3495943: Handle module preprocess functions as OOP hooks should make this pretty straight-forward. Once we converted the remaining preprocess functions in .inc files in core like system and views, we should be able to trigger deprecations if this is set.

nicxvan’s picture

nicxvan CreditAttribution: nicxvan commented

ThemeCommonElements uses it too, but that should be able to be cleaned up too.

berdir opened merge request !11808

berdir’s picture

berdir
Primary language German
Location Switzerland
CreditAttribution: berdir at MD Systems GmbH commented
Title: Remove 'includes' support from hook_theme() » Deprecate 'includes' and 'file' support from hook_theme()

Still postponed, but wanted to have a try, all we have to do now is trigger a deprecation if one of the two keys is set, started a MR based on #3504381: [meta] Convert Template Preprocess hooks to OOP equivalent which removes those in system module and a quickfix for authorize report that I might move to #3518822: Convert template preprocess hooks in core/includes/theme.inc, there shouldn't be too much left after that.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Postponed » Active

Blocker has been committed

berdir’s picture

berdir
Primary language German
Location Switzerland
CreditAttribution: berdir at MD Systems GmbH commented
Status: Active » Postponed

Still postponed on at 50% of #3504381: [meta] Convert Template Preprocess hooks to OOP equivalent, template_preprocess is actually the main thing here, module preprocess aren't really in those files. I'm aggregating the relevant ones from the meta to check what's left, but keeping at postponed.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Related issues: +#2538970: Improve the speed of \Drupal\Core\Theme\ThemeAccessCheck, +#2531958: Try to do less in theme_get_settings() on every request
berdir’s picture

berdir
Primary language German
Location Switzerland
CreditAttribution: berdir at MD Systems GmbH commented

This is still postponed and contains 4 or so other issues and a bunch of manual fixes, but with all that, it is now green. We might need to add a BC test though for this. could be combined with a BC test for template_preprocess.