Make WordPress Core

Opened 6 years ago

Last modified 6 years ago

#42513 new defect (bug)

WP_Theme::get_post_templates() is extremely inefficient for large themes

Reported by: gschoppe's profile gschoppe Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.8.3
Component: Themes Keywords: has-patch needs-unit-tests
Focuses: administration, 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)

refactor_class_wp_theme_get_post_templates.patch (2.4 KB) - added by gschoppe 6 years ago.
Patch refactoring WP_Theme::get_post_templates() to use get_file_data

Download all attachments as: .zip

Change History (5)

@gschoppe
6 years ago

Patch refactoring WP_Theme::get_post_templates() to use get_file_data

#1 @gschoppe
6 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 @birgire
6 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 @gschoppe
6 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 @gschoppe
6 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

Note: See TracTickets for help on using tickets.