WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 6 weeks ago

#51691 closed enhancement (wontfix)

Call get_template_directory() once instead of multiple times for loading theme's files (performance boost)

Reported by: hellofromTonya Owned by:
Milestone: Priority: normal
Severity: normal Version: 1.5
Component: Bundled Theme Keywords:
Focuses: performance Cc:

Description (last modified by hellofromTonya)

In the bundled themes, get_template_directory() is invoked for loading each of the theme's files, i.e. in the theme's functions.php file.

This degrades the performance of the theme's initialization. How so? Each time get_template_directory() runs, multiple other functions and filters also run including these 5 hooks:

  • pre_option_template
  • alloptions
  • option_template
  • theme_root
  • template_directory

The number of times get_template_directory() is called for loading theme files:

  • Twenty Twenty One: 12 times
  • Twenty Twenty: 12 times
  • Twenty Nineteen: 10 times
  • etc.

This ticket proposes to call get_template_directory() only once for loading of theme's local files.

There are different ways to achieve this a performance gain.

One way:

  • replacing the get_template_directory() with a private memoized theme function
  • calling get_template_directory() only once and then storing it into a static variable for reuse within the private function

Another way:

  • Store in a variable and reuse

Another way:

  • Implement within Core

The overall goal is to improve performance by reducing memory, CPU cycles, and processing time.

Change History (16)

#1 @joyously
4 months ago

I have reviewed themes in which they use a constant or theme function to do this, but it goes against the guidelines, which say to use core functions if they exist. The reason is that those filters are run!
I once wrote a child theme for a theme that loaded a file in the setup function. I wanted everything except that file, so I filtered get_template_directory and loaded the child version instead of the parent version. If that theme had used a theme function that cached it instead of a core function, I couldn't have filtered it and made it work.

If any enhancements are done, it should happen in the get_template_* functions themselves, instead of having themes change to another function to be more performant (but less "the WordPress way").

#2 follow-up: @audrasjb
4 months ago

Hello @hellofromTonya and thank you for opening this ticket,

While I understand the performance benefits of this change, I personally don't think it's a good idea for bundled themes.

Bundled themes are supposed to be a place where developers can learn how themes should ideally work. Replacing basic functions like get_template_directory() with custom stuff would be good for performances, but bad in terms of training/learning. Also, the five hooks you mentioned are here for a reason. I think removing them on existing bundled theme is not a good option as it may introduce backward compatibility issues with plugins and custom developments that use those functions/hooks to override the defaults settings.

But that's my personal opinion about this change. Let's discuss it :)

Version 0, edited 4 months ago by audrasjb (next)

#3 in reply to: ↑ 2 @hellofromTonya
4 months ago

Replying to audrasjb:

but bad in terms of training/learning.

How so? Shouldn't we also be teaching more performant approaches to building themes?

Replying to audrasjb:

Also, the five hooks you mentioned are here for a reason. I think removing them on existing bundled theme is not a good option as it may introduce backward compatibility issues with plugins and custom developments that use those functions/hooks to override the defaults settings.

The file's path/filename is not passed to the function. What is the advantage of having these filters run for each theme file that is loaded? curious

#4 @davidbaumwald
4 months ago

  • Type changed from task (blessed) to enhancement

Hi, @hellofromTonya, and welcome to Trac! 😉

Whether or not this gets accepted, I think it's technically an Enhancement as it's not a stated goal for an upcoming release.

#5 @hellofromTonya
4 months ago

  • Description modified (diff)

Modified the description to focus it on the performance objective, rather than the implementation itself. There are many ways to achieve the performance objective.

Last edited 4 months ago by hellofromTonya (previous) (diff)

#6 follow-up: @apedog
4 months ago

There's an assumption here - that get_template_directory() should return the same value every time it's called by the theme. If so, the result can be memoized and re-used. That's a big assumption for a default theme that's meant to be extended in a child-theme by users.

12 calls to 5 hooks that might not all be filtered, and if filtered, will likely involve a simple string replacement, is not a lot of performance. - I think.

Adding this "cached" result is actually a restriction for the user extending the theme.

#7 in reply to: ↑ 6 @hellofromTonya
4 months ago

Replying to apedog:

There's an assumption here - that get_template_directory() should return the same value every time it's called by the theme. If so, the result can be memoized and re-used. That's a big assumption for a default theme that's meant to be extended in a child-theme by users.

That's true. You're right. It is an assumption.

That makes me wonder about the original design intent.

Is the design intent to allow each file to be loaded separately from separate different directory sources? In other words, it's intentional to allow the first file to be loaded from say the theme, 2nd file from the child theme, 3rd from a plugin, etc.

<?php
require get_template_directory() . '/classes/class-twenty-twenty-one-svg-icons.php';

// Custom color classes.
require get_template_directory() . '/classes/class-twenty-twenty-one-custom-colors.php';
new Twenty_Twenty_One_Custom_Colors();

// Enhance the theme by hooking into WordPress.
require get_template_directory() . '/inc/template-functions.php';

// Menu functions and filters.
require get_template_directory() . '/inc/menu-functions.php';

// Custom template tags for the theme.
require get_template_directory() . '/inc/template-tags.php';

// Customizer additions.
require get_template_directory() . '/classes/class-twenty-twenty-one-customize.php';
new Twenty_Twenty_One_Customize();

// Block Patterns.
require get_template_directory() . '/inc/block-patterns.php';

// Block Styles.
require get_template_directory() . '/inc/block-styles.php';

Or is the design intent to filter the source directory where all of the above files will be loaded from?

#8 follow-up: @apedog
4 months ago

Yeah. Filters are often conditional.
The design is to allow a user to short-circuit/override the default output in a filter. Either wholesale (whenever the hook is called) or only when a certain condition is met.

#9 in reply to: ↑ 8 @hellofromTonya
4 months ago

Replying to apedog:

Yeah. Filters are often conditional.
The design is to allow a user to short-circuit/override the default output in a filter. Either wholesale (whenever the hook is called) or only when a certain condition is met.

So you're saying, that yes, the theme is intentionally designed to allow each file to be loaded from different directory source? More concretely from the code shared above, the design allows each of the 8 different files to be loaded from 8 different directory sources.

If yes, that seems hard to control through a callback. Why? How would the callback know with certainty which file belongs to the get_template_directory() being called?

#10 @apedog
4 months ago

I'm saying WordPress core allows a user to manipulate the output of get_template_directory(). And that the theme cannot assume get_template_directory() will/should return the same output on every call.

#11 in reply to: ↑ description @SergeyBiryukov
4 months ago

Replying to hellofromTonya:

Each time get_template_directory() runs, multiple other functions and filters also run including these 5 hooks:

  • pre_option_template
  • alloptions
  • option_template
  • theme_root
  • template_directory

If any of these have specific performance concerns, I think a better way forward for this ticket might be to explore adding some object caching or static variables to minimize the overhead.

I think a theme should be able to call get_template_directory() multiple times without a noticeable performance impact. So I would go with the final option here:

Another way:

  • Implement within Core

The overall goal is to improve performance by reducing memory, CPU cycles, and processing time.

This ticket was mentioned in Slack in #core by noisysocks. View the logs.


4 months ago

#13 @noisysocks
4 months ago

Some benchmarking data would be useful here. What's the total amount of time taken to make all of these calls to get_template_directory()?

#14 @hellofromTonya
4 months ago

@noisysocks You're right. Benchmarking is the next step. It will give us the data we need to make decisions. Adding that to my TODO list.

#15 @hellofromTonya
4 months ago

  • Version changed from trunk to 1.5

#16 @hellofromTonya
6 weeks ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Closing due to lack of interest.

Note: See TracTickets for help on using tickets.