-
Notifications
You must be signed in to change notification settings - Fork 3.2k
This code introduces a caching mechanism to the sanitize method wit…
#8213
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
base: trunk
Are you sure you want to change the base?
This code introduces a caching mechanism to the sanitize method wit…
#8213
Conversation
…hin the `WP_Theme_JSON` class. Let's break down the changes and their impact. **Problem:** The original `sanitize` method likely performed redundant computations when called multiple times with the same input. Sanitizing theme JSON data can be resource-intensive, especially with complex themes or frequent calls. **Solution:** The added code addresses this inefficiency by implementing a simple caching mechanism. Before performing the sanitization, the method now checks if the input has already been processed. **How the Cache Works:** 1. **Hashing the Input:** The `md5` function generates a unique hash of the input array using `wp_json_encode`. This hash serves as the key for the cache. Using a hash ensures fast lookups and avoids potential issues with complex array keys. 2. **Checking the Cache:** The code checks if the generated hash key exists in the `$sanitize_input_cache` static property. If found, the cached sanitized output is returned directly, bypassing the sanitization process. 3. **Caching the Result:** If the hash is not found in the cache, the sanitization proceeds as before. After the sanitization is complete, the result is stored in the `$sanitize_input_cache`, associating it with the calculated hash key. **Benefits of this Change:** * **Performance Improvement:** By avoiding redundant calculations, this caching significantly improves performance, especially in scenarios where the same theme JSON data is sanitized repeatedly. * **Reduced Resource Consumption:** Less processing means reduced CPU load and memory usage. **Potential Considerations:** * **Cache Invalidation:** The current implementation doesn't have a mechanism for cache invalidation. This means that if the underlying theme JSON data changes, the cached value might become stale. A more robust solution might consider ways to invalidate the cache when theme changes occur. * **Memory Usage:** While generally beneficial, caching can consume memory. If the number of unique theme JSON variations is extremely large, the cache could potentially grow quite large. Monitoring memory usage is advisable, especially in memory-constrained environments. **Overall, this change is a positive improvement that addresses performance concerns in theme JSON sanitization.** The simple caching strategy effectively reduces redundant processing and enhances efficiency. However, it's important to be aware of the potential need for cache invalidation in a production environment.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @[email protected]. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
…l to `WP_Theme_JSON::reset_sanitize_input_cache()` before setting the transient containing global styles and settings. This change likely addresses a caching issue where modifications to theme.json weren't being reflected because the sanitized input was cached. By resetting the sanitization cache before updating the transient, the updated theme.json settings will be correctly sanitized and saved, ensuring the changes take effect. This implies that prior to this change, the sanitized values were being retrieved from the cache, even if the theme.json file had been modified. This fix ensures that the transient always contains the most up-to-date sanitized styles and settings.
…l to `WP_Theme_JSON::reset_sanitize_input_cache()` before setting the transient containing global styles and settings. This change likely addresses a caching issue where modifications to theme.json weren't being reflected because the sanitized input was cached. By resetting the sanitization cache before updating the transient, the updated theme.json settings will be correctly sanitized and saved, ensuring the changes take effect. This implies that prior to this change, the sanitized values were being retrieved from the cache, even if the theme.json file had been modified. This fix ensures that the transient always contains the most up-to-date sanitized styles and settings.
adamsilverstein
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, small points of feedback
Co-authored-by: Weston Ruter <[email protected]>
Co-authored-by: Weston Ruter <[email protected]>
Co-authored-by: Adam Silverstein <[email protected]>
Co-authored-by: Weston Ruter <[email protected]>
…_Theme_JSONsanitize
Co-authored-by: Mukesh Panchal <[email protected]>
…anitization, caching behavior, and handling of valid/invalid elements.
…y in `Tests_ThemeJsonSanitize`.
…y in `Tests_ThemeJsonSanitize`.
…y in `Tests_ThemeJsonSanitize`.
Co-authored-by: Weston Ruter <[email protected]>
Co-authored-by: Weston Ruter <[email protected]>
Co-authored-by: Weston Ruter <[email protected]>
Co-authored-by: Weston Ruter <[email protected]>
Co-authored-by: Weston Ruter <[email protected]>
|
Code looks good to me, nice work here @pbearne! |
…_Theme_JSONsanitize
westonruter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adjuested cache key
…hin the
WP_Theme_JSONclass. Let's break down the changes and their impact.Problem: The original
sanitizemethod likely performed redundant computations when called multiple times with the same input. Sanitizing theme JSON data can be resource-intensive, especially with complex themes or frequent calls.Solution: The added code addresses this inefficiency by implementing a simple caching mechanism. Before performing the sanitization, the method now checks if the input has already been processed.
How the Cache Works:
Hashing the Input: The
md5function generates a unique hash of the input array usingwp_json_encode. This hash serves as the key for the cache. Using a hash ensures fast lookups and avoids potential issues with complex array keys.Checking the Cache: The code checks if the generated hash key exists in the
$sanitize_input_cachestatic property. If found, the cached sanitized output is returned directly, bypassing the sanitization process.Caching the Result: If the hash is not found in the cache, the sanitization proceeds as before. After the sanitization is complete, the result is stored in the
$sanitize_input_cache, associating it with the calculated hash key.Benefits of this Change:
Potential Considerations:
Overall, this change is a positive improvement that addresses performance concerns in theme JSON sanitization. The simple caching strategy effectively reduces redundant processing and enhances efficiency. However, it's important to be aware of the potential need for cache invalidation in a production environment.
Trac ticket: https://core.trac.wordpress.org/ticket/62126