Skip to content

Conversation

@pbearne
Copy link

@pbearne pbearne commented Jan 29, 2025

…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.

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

…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.
@github-actions
Copy link

github-actions bot commented Jan 29, 2025

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 props-bot label.

Unlinked Accounts

The 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:

Props pbearne, adamsilverstein, westonruter, mukesh27.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions
Copy link

Test using WordPress Playground

The 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

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

Paul Bearne added 2 commits January 29, 2025 11:19
…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.
@joemcgill joemcgill self-requested a review January 29, 2025 17:47
Copy link
Member

@adamsilverstein adamsilverstein left a 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

@adamsilverstein
Copy link
Member

Code looks good to me, nice work here @pbearne!

@pbearne pbearne requested a review from westonruter September 25, 2025 17:31
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

adjuested cache key
@pbearne pbearne requested a review from westonruter September 26, 2025 15:26
@westonruter
Copy link
Member

@pbearne How will changing the serialization method 09b327b improve the underlying issue with the low HIT rate?

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.

4 participants