Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove memoization #5644

Closed

Conversation

joemcgill
Copy link
Member

An alternative approach to #5642 is to remove the memoization completely, which reverts these functions to their 6.3 behavior.

Trac ticket: https://core.trac.wordpress.org/ticket/59847


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@joemcgill joemcgill force-pushed the fix/59847-remove-memoization branch from 57c5e02 to 111de9f Compare November 9, 2023 15:10
@joemcgill joemcgill requested a review from felixarntz November 9, 2023 15:55
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joemcgill Code-wise this looks good. Could you do a profiling / benchmarking check on whether/how much this slows things down? If it's negligible, I think we should go with this change as it would be the safer one to make over #5642. Otherwise, we can try exploring whether #5642 is enough.

@joemcgill
Copy link
Member Author

@felixarntz I can do some benchmarks, but to be clear, this just restores these functions to their previous behavior, so it should have no impact when compared with 6.3 and previous versions.

@felixarntz
Copy link
Member

@joemcgill Yes but part of why the memorizing was introduced was because these functions are now being called more often (rather than checking a constant). So there's a chance this change could make performance worse than pre-WP6.4. I would assume the impact is negligible but we should verify.

@joemcgill
Copy link
Member Author

@felixarntz the performance test summary for this PR shows very little change between this and trunk. However, given that those results can fluctuate, I ran local benchmarks of this branch vs trunk and got the following server timing numbers:

metric trunk PR diff diff%
wp-total (p10) 311.71 313.89 2.18 0.70%
wp-total (p25) 313.7 315.34 1.64 0.52%
wp-total (p50) 317 317.86 0.86 0.27%
wp-total (p75) 319.7 320.75 1.05 0.33%
wp-total (p90) 326.99 324.57 -2.42 -0.74%

Seems like a safe change to me.

Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Left some nit-pick

src/wp-includes/theme.php Outdated Show resolved Hide resolved
src/wp-includes/theme.php Outdated Show resolved Hide resolved
@joemcgill
Copy link
Member Author

Added some unit tests to this PR that demonstrate the problems in trunk but now pass with this PR. The second set are tricky to show in unit tests because you have to switch to set the application state to where a theme is active from an alternate theme root directory that is not yet registered when the functions are called the first time, so there's a bit of filter mocking going on in the tests. Would be happy for feedback on ways of improving those tests, if anyone has ideas.

@joemcgill joemcgill force-pushed the fix/59847-remove-memoization branch from 473953e to 0b210de Compare November 20, 2023 14:47
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @joemcgill, tests look solid!

@joemcgill
Copy link
Member Author

Merged in https://core.trac.wordpress.org/changeset/57129.

@joemcgill joemcgill closed this Nov 20, 2023
@joemcgill joemcgill deleted the fix/59847-remove-memoization branch October 11, 2024 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants