Opened 7 years ago
Last modified 2 months ago
#42513 new defect (bug)
WP_Theme::get_post_templates() is extremely inefficient for large themes
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | 4.8.3 |
Component: | Themes | Keywords: | good-first-bug has-patch |
Focuses: | performance | Cc: |
Description
WP_Theme::get_post_templates() uses file_get_contents() to read the complete contents of each theme php file in the root of a theme into memory, twice, and processes the result with a custom Regular Expression.
As a result, significantly more data than necessary is processed when loading large themes.
In addition, because the standard get_file_data() function is not called (as it is with all other header comments), the "extra_{$context}_headers" filter is never called, which is inconsistent behavior with other file_header related operations, such as the WP_Theme constructor.
Attachments (1)
Change History (8)
#1
@
7 years ago
To make it easier to review the issue without having to create a server and simulate high load, I've created a pathological theme that, when activated, should demonstrate the increased loading time. The file size was too large to upload directly to this ticket, so I have hosted it on google drive:
https://drive.google.com/open?id=1t0S_yErQXQJZGlgDBu_Uo9UP3xIJvVJb
The load time should be significant.
#2
@
7 years ago
- Keywords has-patch needs-unit-tests added
@gschoppe Welcome to WordPress trac.
This seems like a good catch.
This will probably need a unit test as well. There's one already for the get_post_templates()
method in Tests_Admin_includesTheme::test_get_post_templates_child_theme()
so maybe it would cover this as well, instead of a new one?
I think I've spotted another part that could benefit from this, namely the get_file_description()
function, that contains:
$template_data = implode( '', file( $file_path ) );
where file()
reads the entire file into an array.
The theme editor uses (4.9+) wp_print_theme_file_tree()
recursively and therefore we have multiple get_file_description()
function calls.
So with your suggestion of using get_file_data()
it seems that we could adjust get_file_description()
as well to be more efficient. Best create a new ticket for that, if this is the way to go here.
#3
@
7 years ago
In the process of writing unit tests, I discovered that get_file data() seems to not support single line file header declarations, such as
<?php // Template Name: Test ?>
This may be why the code was not previously using get_file_data(). I am now looking into whether it is feasible to add support for single line headers to get_file_data() as this lack of support seems to be an oversight, and this format needs uniform syntax across all use cases.
#4
@
7 years ago
I have opened a related ticket to refactor the regex used to parse file header comments, so that get_file_data will support the single line variant.
Related Ticket: #42517
This ticket was mentioned in Slack in #core-performance by swissspidy. View the logs.
2 months ago
#6
@
2 months ago
- Focuses administration removed
- Keywords needs-patch good-first-bug added; has-patch needs-unit-tests removed
- Milestone changed from Awaiting Review to Future Release
Not too worried about the performance impact here as the result is stored in cache.
Being able to use get_file_data() is blocked by #42517.
Calling file_get_contents()
only once would be a trivial change that makes sense to do.
This ticket was mentioned in PR #8310 on WordPress/wordpress-develop by @sukhendu2002.
2 months ago
#7
- Keywords has-patch added; needs-patch removed
Trac ticket: https://core.trac.wordpress.org/ticket/42513
Patch refactoring WP_Theme::get_post_templates() to use get_file_data