Add cache pool abstraction to cache stylesheets #3300

schlessera · 2019-09-19T06:26:55Z

This PR adds a cache pool abstraction that abstracts away the differences between the object cache and transients when caching stylesheets.

The transient cache implementation provides a fixed-size pool of cache entries that are rotated to enforce a maximum number of transients and avoid filling up the database.

Note: For the architecture rethink, this should be an interface with 2 separate implementations that will get injected, instead of a single class that contains all implementations.

See #2092

kienstra · 2019-09-19T20:16:53Z

Approved

Hi @schlessera,
This looks really good.

Once a parsed stylesheet is stored in a transient, this retrieves it from the transient, instead of parsing it again.

I set an Xdebug breakpoint and stepped through $this->cache_pool->set( $cache_key, $parsed ), and it looked to work as excepted.

kienstra · 2019-09-19T19:23:08Z

includes/sanitizers/class-amp-style-sanitizer.php

@@ -399,6 +418,7 @@ public function __construct( DOMDocument $dom, array $args = [] ) {
 		$this->base_url    = untrailingslashit( $guessurl );
 		$this->content_url = WP_CONTENT_URL;
 		$this->xpath       = new DOMXPath( $dom );
+		$this->cache_pool  = new AMP_Cache_Pool( self::CACHE_GROUP );


Nice dependency injection 😄

This is not being injected because I don't think the architecture is currently set up well for injecting dependencies.
For now, I wanted to keep it simple and just directly instantiated here. Hopefully, we'll have injection for v2.

Ah, OK. I thought this was dependency injection.

Dependency injection would be if exterior code would inject "some cache" and this code here would not need to know which exact implementation is being used. This would then decouple this code from the cache, and the cache could be changed/replaced without needing to adapt this code here.

OK, thanks!

kienstra · 2019-09-19T19:30:07Z

includes/utils/class-amp-cache-pool.php

+	 * @param string $group Optional. Group to use. Defaults to an empty string.
+	 * @param int    $size  Optional. Size of the cache pool.
+	 */
+	public function __construct( $group = '', $size = self::DEFAULT_POOL_SIZE ) {


It looks like so far, this class is only instantiated with one argument:

$this->cache_pool = new AMP_Cache_Pool( self::CACHE_GROUP );

...though maybe later this class will be instantiated with 2 arguments.

The size is needed, and it felt obvious to allow this to be set through instantiation. This way, you could also create several pools with differing groups that might have different sizes as well.

kienstra · 2019-09-19T19:31:58Z

tests/php/test-class-amp-cache-pool.php

+ */
+class Test_AMP_Cache_Pool extends WP_UnitTestCase {
+
+	public function test_transients_storing_a_value() {


Wow, very thorough testing 😄

kienstra · 2019-09-19T20:13:59Z

includes/sanitizers/class-amp-style-sanitizer.php

-		} else {
-			$parsed = get_transient( $cache_group . '-' . $cache_key );
-		}
+		$parsed    = $this->cache_pool->get( $cache_key );


This looks to work well.

Once the stylesheet is stored in a transient, the ->get() method retrieves it.

westonruter

This looks great. Left a few comments. This may make sense to also use in the AMP_Image_Dimension_Extractor since images can also end up filling up the wp_options table.

westonruter · 2019-09-20T17:21:46Z

includes/utils/class-amp-cache-pool.php

+		$this->group = $group;
+		$this->size  = $size;
+
+		$this->is_object_cache_available = wp_using_ext_object_cache();


If using an external object cache, then essentially the cache pool is not used, is that right? This assumes that that the persistent object cache has implemented some algorithm for garbage collection.

Aside: Shouldn't WordPress transient system implement some cache replacement policy? If there was such a cache pool in core like you've implemented here, then there wouldn't be a need for this class, right?

Yes, the manual pooling/rotation is only used with transients. Proper object caches have built-in mechanisms for this.

I think WordPress should use something like this with a separate table, transients as they are now are a hack with several performance issues.

includes/utils/class-amp-cache-pool.php

includes/sanitizers/class-amp-style-sanitizer.php

westonruter · 2019-09-20T17:32:34Z

includes/sanitizers/class-amp-style-sanitizer.php

-				// The expiration is to ensure transient doesn't stick around forever since no LRU flushing like with external object cache.
-				set_transient( $cache_group . '-' . $cache_key, $parsed, MONTH_IN_SECONDS );
-			}
+			$this->cache_pool->set( $cache_key, $parsed );


This is great. 👍

westonruter · 2019-09-20T18:03:43Z

includes/utils/class-amp-cache-pool.php

+		self::$pool_indexes[ $this->group ]++;
+
+		if ( self::$pool_indexes[ $this->group ] >= $this->size ) {
+			self::$pool_indexes[ $this->group ] = 0;


In regards to #2092 (comment), this would be where we could potentially show an error message to the user, warning the that stylesheet cache pool is overflowing, yes? This should issue a PHP warning to the error log?

No, this is only expressing the "distance travelled". Velocity is distance over time, we don't know anything about the time here. If we hit the end of the pool several times a day, we have a very high velocity. If we only hit it every 3 months, it might just be a site with lots of edits.

Co-Authored-By: Weston Ruter <westonruter@google.com>

schlessera · 2019-09-20T20:46:55Z

This may make sense to also use in the AMP_Image_Dimension_Extractor since images can also end up filling up the wp_options table.

Yes, it was already built to be usable in multiple locations, with differing groups and sizes, and the tests already make sure this works as expected.

Co-Authored-By: Weston Ruter <westonruter@google.com>

schlessera · 2019-09-20T20:52:44Z

I wonder whether the default size of 1000 pool entries (per group) is not too high. The main issue I can see is that we serialize and unserialize the entire pool map. For 1000 entries, this might have a non-negligible performance impact.

westonruter · 2019-09-20T22:34:38Z

It's hard to know. We somewhat need data to help inform that.

This provides the count:

wp transient list --fields=name --search='amp-parsed-stylesheet-*' --format=count

I've asked someone to run that to get a number from a production site that is not running an object cache.

westonruter · 2019-09-20T22:37:08Z

Note that a the parsed stylesheets being stored here can come from:

Stylesheets referenced by link elements.
Stylesheets inline in style elements.
Style properties stored in inline style attributes.

A transient will be created for each 👉 unique 👈 instance of the above to contain the parsed stylesheet information.

schlessera · 2019-09-23T13:27:52Z

On hold for now as we are awaiting an export of production transient data from an actual site to evaluate.

schlessera · 2019-09-26T14:41:35Z

Here's my initial observation from looking through the export we got from a bigger site:

Every value is a serialization of a serialization. Given that most of our memory usage is located in unserialize() according to initial profiling, I recommend we change this so that we only ever have one level of serialization.
The format is not very space-efficient, there's certainly improvements we can do here.
A large majority of the entries are every simple, often just a single property, like flex:0.3456 or text-align:center. I think we should discuss how to store this in a more optimized way and/or introducing utility classes. So, instead of having .amp-wp-4263548 { text-align: center; } .amp-wp-9263742 { text-align: center; } ..., we could have .amp-wp-tacenter { text-align: center; } instead which would deduplicate this. Similarly, we could have "calculated utility classes" that need to be calculated and tracked, but don't need to be stored. So, instead of .amp-wp-3856964 { flex: 2.564; } we could have .amp-wp-flex-2-564 { flex: 2.564; }. This would not deduplicate these (as they are quite individualized), but we would not need to store them in the database.
The fields that are almost always empty should not even be part of the serialization, like 'validation_results' or 'imported_font_urls'.

schlessera · 2019-09-26T14:45:49Z

Also, what is the reasoning behind extracting all the inline styles into <style amp-custom>? As far as I can tell, AMP does support inline styles...

kienstra · 2019-09-26T18:24:24Z

Also, what is the reasoning behind extracting all the inline styles into <style amp-custom>? As far as I can tell, AMP does support inline styles...

Ah, interesting. Maybe that's because AMP used to not support inline styles.

westonruter · 2019-09-27T00:22:20Z

Ah, interesting. Maybe that's because AMP used to not support inline styles.

That's mostly why, but there are other reasons. See #1313 which suggests the elimination of moving inline style attributes to style rules, but this is problematic currently when a style rule contains an !important property, as that is how a style rule can override an inline style, and there's no way to override it otherwise, as far as I know. Please continue this discussion following #1313 (comment).

westonruter · 2019-09-27T00:25:19Z

A large majority of the entries are every simple, often just a single property, like flex:0.3456 or text-align:center. I think we should discuss how to store this in a more optimized way and/or introducing utility classes. So, instead of having .amp-wp-4263548 { text-align: center; } .amp-wp-9263742 { text-align: center; } ..., we could have .amp-wp-tacenter { text-align: center; } instead which would deduplicate this. Similarly, we could have "calculated utility classes" that need to be calculated and tracked, but don't need to be stored. So, instead of .amp-wp-3856964 { flex: 2.564; } we could have .amp-wp-flex-2-564 { flex: 2.564; }. This would not deduplicate these (as they are quite individualized), but we would not need to store them in the database.

These are all coming from inline styles (in style attributes). This being the case, it may be better to parse the style rules at runtime rather than fire up the PHP CSS Parser. This was done recently in #3209 when converting elements for layout=fill. Perhaps this could be done here as well, and this would eliminate a lot of transients. We'd want to ensure the same sanitizations are done on the properties, however.

schlessera · 2019-09-27T15:54:23Z

These are all coming from inline styles (in style attributes). This being the case, it may be better to parse the style rules at runtime rather than fire up the PHP CSS Parser. This was done recently in #3209 when converting elements for layout=fill. Perhaps this could be done here as well, and this would eliminate a lot of transients. We'd want to ensure the same sanitizations are done on the properties, however.

I'm not quite sure I understand what you mean by the above, specifically the suggestion to "parse the style rules at runtime rather than fire up the PHP CSS Parser". Would that mean they are neither validated/sanitized, not tree-shaken anymore? And what would you then actually do with them at runtime? I seem to be missing something here.

westonruter · 2019-09-27T22:55:56Z

The inline styles don't need to be tree-shaken, because we know they are going to exist in the page. What I mean is that in AMP_Style_Sanitizer::collect_inline_styles() it is currently constructing a style rule dynamically like so:

amp-wp/includes/sanitizers/class-amp-style-sanitizer.php

Lines 2437 to 2439 in 72bd441

    
           $class = 'amp-wp-' . substr( md5( $style_attribute->nodeValue ), 0, 7 ); 
        
           $root  = ':root' . str_repeat( ':not(#_)', self::INLINE_SPECIFICITY_MULTIPLIER ); 
        
           $rule  = sprintf( '%s .%s { %s }', $root, $class, $style_attribute->nodeValue );

And then the rule gets processed:

amp-wp/includes/sanitizers/class-amp-style-sanitizer.php

Lines 2443 to 2449 in 72bd441

    
           $processed = $this->process_stylesheet( 
        
           	$rule, 
        
           	[ 
        
           		'allowed_at_rules'   => [], 
        
           		'property_whitelist' => $this->style_custom_cdata_spec['css_spec']['declaration'], 
        
           	] 
        
           );

This then causes the PHP CSS Parser to be fired up. The AMP_Style_Sanitizer::process_stylesheet() takes as input a stylesheet, meaning it has to handle arbitrary style rules. However, the data being processed here is simple style properties. So it seems this could do something faster by skipping the full parsing and instead extract the properties to then do the work in the AMP_Style_Sanitizer::process_css_delcaration_block(), which would be limited to just needing to do:

And I think that's all that needs to be done for properties in a style attribute.

schlessera · 2019-09-28T07:32:50Z

Okay, I understood now, the term "runtime" was confusing me. I'll open a new issue for that.

What is needed now to take this PR across the finish line?

Add size argument to $DEFAULT_ARGS so sites can override our default.
Add documentation about the transient pool and how to adapt its size.

Would this be enough to merge this for now?

westonruter · 2019-09-28T15:27:23Z

Given that a large majority of the transients are for inline styles, if the PHP CSS Parser were bypassed in favor of a more lightweight parser (e.g. using regex, though this is less robust!) then we could avoid needing to store so many transients and there will be less need to increase the size of the pool.

westonruter · 2019-09-28T15:28:14Z

I'm just concerned that if a site has 5000 transients and the pool size is 1000, but they don't know they have so many, then there will be very frequent cache misses and they wouldn't know it is happening.

archon810 · 2019-09-28T15:35:08Z

This 👆.

westonruter · 2019-09-28T15:38:26Z

I wonder if the PHP CSS Parser is actually sufficiently fast for the simple case if CSS properties in a style attribute.

westonruter · 2019-09-28T15:52:03Z

Also because there is no need to parse the selector for tree shaking.

westonruter · 2020-03-23T07:06:53Z

Closing in favor of #4177

schlessera added 5 commits September 19, 2019 08:23

Add rotating cache pool

4ae843d

Add limiting size to cache pool

9530125

Fix cache pool indexing

9217414

Make map and index static and store them by group

f980e52

Add tests

8185a90

googlebot added the cla: yes Signed the Google CLA label Sep 19, 2019

schlessera added the Bug Something isn't working label Sep 19, 2019

schlessera requested review from westonruter and kienstra September 19, 2019 06:27

schlessera mentioned this pull request Sep 19, 2019

Detect high cache miss rate for obtaining parsed CSS from variable stylesheets #2092

Closed

Fix alignment

d4d3a11

kienstra approved these changes Sep 19, 2019

View reviewed changes

westonruter reviewed Sep 20, 2019

View reviewed changes

Update includes/utils/class-amp-cache-pool.php

1a92bd2

Co-Authored-By: Weston Ruter <westonruter@google.com>

Pass in pool size through $this->args

323edc7

Co-Authored-By: Weston Ruter <westonruter@google.com>

westonruter approved these changes Sep 20, 2019

View reviewed changes

westonruter closed this Mar 23, 2020

schlessera deleted the fix/2092-rotate-stylesheet-cache branch November 6, 2020 15:56

Add cache pool abstraction to cache stylesheets #3300

Add cache pool abstraction to cache stylesheets #3300

Conversation

schlessera commented Sep 19, 2019

kienstra commented Sep 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schlessera Sep 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra Sep 19, 2019 • edited Loading

Choose a reason for hiding this comment

westonruter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schlessera commented Sep 20, 2019 • edited Loading

schlessera commented Sep 20, 2019

westonruter commented Sep 20, 2019

westonruter commented Sep 20, 2019

schlessera commented Sep 23, 2019

schlessera commented Sep 26, 2019 • edited Loading

schlessera commented Sep 26, 2019

kienstra commented Sep 26, 2019

westonruter commented Sep 27, 2019

westonruter commented Sep 27, 2019

schlessera commented Sep 27, 2019

westonruter commented Sep 27, 2019 • edited Loading

schlessera commented Sep 28, 2019

westonruter commented Sep 28, 2019

westonruter commented Sep 28, 2019

archon810 commented Sep 28, 2019

westonruter commented Sep 28, 2019

westonruter commented Sep 28, 2019

westonruter commented Mar 23, 2020

schlessera Sep 20, 2019 •

edited

Loading

kienstra Sep 19, 2019 •

edited

Loading

schlessera commented Sep 20, 2019 •

edited

Loading

schlessera commented Sep 26, 2019 •

edited

Loading

westonruter commented Sep 27, 2019 •

edited

Loading