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

Add cache pool abstraction to cache stylesheets #3300

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions includes/class-amp-autoloader.php
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ class AMP_Autoloader {
'AMP_Content_Sanitizer' => 'includes/templates/class-amp-content-sanitizer',
'AMP_Post_Template' => 'includes/templates/class-amp-post-template',
'AMP_DOM_Utils' => 'includes/utils/class-amp-dom-utils',
'AMP_Cache_Pool' => 'includes/utils/class-amp-cache-pool',
'AMP_HTML_Utils' => 'includes/utils/class-amp-html-utils',
'AMP_Image_Dimension_Extractor' => 'includes/utils/class-amp-image-dimension-extractor',
'AMP_Validation_Manager' => 'includes/validation/class-amp-validation-manager',
Expand Down
45 changes: 24 additions & 21 deletions includes/sanitizers/class-amp-style-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,18 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer {
*/
const SELECTOR_EXTRACTED_ATTRIBUTES = 3;

/**
* Cache group to use for caching stylesheets.
*
* This should be bumped whenever the PHP-CSS-Parser is updated or parsed format is updated.
*
* @var string
* @private
* @since 1.4
* @see \AMP_Style_Sanitizer::prepare_stylesheet()
*/
const CACHE_GROUP = 'amp-parsed-stylesheet-v19';

/**
* Array of flags used to control sanitization.
*
Expand Down Expand Up @@ -304,6 +316,13 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer {
'amp-youtube',
];

/**
* Cache pool implementation to use if no object cache is available.
*
* @var AMP_Cache_Pool;
*/
private $cache_pool;

/**
* Get error codes that can be raised during parsing of CSS.
*
Expand Down Expand Up @@ -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, isset( $this->args['cache_pool_size'] ) ? $this->args['cache_pool_size'] : AMP_Cache_Pool::DEFAULT_POOL_SIZE );
}

/**
Expand Down Expand Up @@ -1348,9 +1368,8 @@ private function fetch_external_stylesheet( $url ) {
* }
*/
private function process_stylesheet( $stylesheet, $options = [] ) {
$parsed = null;
$cache_key = null;
$cache_group = 'amp-parsed-stylesheet-v19'; // This should be bumped whenever the PHP-CSS-Parser is updated or parsed format is updated.
$parsed = null;
$cache_key = null;

$cache_impacting_options = array_merge(
wp_array_slice_assoc(
Expand All @@ -1367,12 +1386,7 @@ private function process_stylesheet( $stylesheet, $options = [] ) {
);

$cache_key = md5( $stylesheet . wp_json_encode( $cache_impacting_options ) );

if ( wp_using_ext_object_cache() ) {
$parsed = wp_cache_get( $cache_key, $cache_group );
} else {
$parsed = get_transient( $cache_group . '-' . $cache_key );
}
$parsed = $this->cache_pool->get( $cache_key );
Copy link
Contributor

@kienstra kienstra Sep 19, 2019

Choose a reason for hiding this comment

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

This looks to work well.

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

sanitizer-here


/*
* Make sure that the parsed stylesheet was cached with current sanitizations.
Expand All @@ -1390,18 +1404,7 @@ private function process_stylesheet( $stylesheet, $options = [] ) {

if ( ! $parsed || ! isset( $parsed['stylesheet'] ) || ! is_array( $parsed['stylesheet'] ) ) {
$parsed = $this->prepare_stylesheet( $stylesheet, $options );

/*
* When an object cache is not available, we cache with an expiration to prevent the options table from
* getting filled infinitely. On the other hand, if an external object cache is available then we don't
* set an expiration because it should implement LRU cache expulsion policy.
*/
if ( wp_using_ext_object_cache() ) {
wp_cache_set( $cache_key, $parsed, $cache_group );
} else {
// 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 );
Copy link
Member

Choose a reason for hiding this comment

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

This is great. 👍

}

return $parsed;
Expand Down
170 changes: 170 additions & 0 deletions includes/utils/class-amp-cache-pool.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,170 @@
<?php
/**
* Class AMP_Cache_Pool
*
* @package AMP
*/

/**
* Cache pool abstraction based on WordPress transients.
*
* @since 1.4.0
*/
final class AMP_Cache_Pool {

/**
* Default size of the cache pool.
*
* @var int
*/
const DEFAULT_POOL_SIZE = 1000;

/**
* Pool map of cached entry keys.
*
* @var array
*/
private static $pool_maps = [];

/**
* Index into the pool map.
*
* @var int
*/
private static $pool_indexes = [];

/**
* Cache group to use.
*
* @var string
*/
private $group;

/**
* Size of the cache pool.
*
* @var int
*/
private $size;

/**
* Whether an object cache is available.
*
* @var bool
*/
private $is_object_cache_available;

/**
* Instantiate an AMP_Cache_Pool object.
*
* @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 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

$this->group = $group;
$this->size = $size;

$this->is_object_cache_available = wp_using_ext_object_cache();
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


if ( ! $this->is_object_cache_available ) {
$this->read_pool_meta();
}
}

/**
* Get the value of a given key from the cache.
*
* @param string $key Key of the cached value to retrieve.
*
* @return mixed Value that was stored under the requested key.
*/
public function get( $key ) {
return $this->is_object_cache_available
? wp_cache_get( $key, $this->group )
: $this->get_rotated_transient( "{$this->group}-{$key}" );
}

/**
* Store a value under a given key in the cache.
*
* @param string $key Key under which to store the value.
* @param mixed $value Value to store in the cache.
*/
public function set( $key, $value ) {
if ( $this->is_object_cache_available ) {
wp_cache_set( $key, $value, $this->group );
} else {
$this->set_rotated_transient( "{$this->group}-{$key}", $value );
}
}

/**
* Get a value from a rotating transient pool.
*
* @param string $key Key of the value to get.
* @return mixed Value for the requested key.
*/
private function get_rotated_transient( $key ) {
$pool_index = array_search( $key, self::$pool_maps[ $this->group ], true );

if ( false === $pool_index ) {
return false;
}

return get_transient( "{$this->group}-pool-slot-{$pool_index}" );
}

/**
* Store a value in the rotating transient pool under a given key.
*
* @param string $key Key under which to store the value.
* @param mixed $value Value to store under the given key.
*/
private function set_rotated_transient( $key, $value ) {
$pool_index = array_search( $key, self::$pool_maps[ $this->group ], true );

// We already have the provided key and the value seems unchanged.
if ( false !== $pool_index && $value === self::$pool_maps[ $this->group ][ $pool_index ] ) {
return;
}

// As we didn't find the key, we create a new pool slot to store it.
if ( false === $pool_index || -1 === self::$pool_indexes[ $this->group ] ) {
$this->advance_pool_index();
self::$pool_maps[ $this->group ][ self::$pool_indexes[ $this->group ] ] = $key;
}

// The expiration is to ensure transients don't stick around forever
// since no LRU flushing like with external object cache.
set_transient( "{$this->group}-pool-slot-" . self::$pool_indexes[ $this->group ], $value, MONTH_IN_SECONDS );

$this->persist_pool_meta();
}

/**
* Read the pool meta information that was persisted.
*/
private function read_pool_meta() {
self::$pool_maps[ $this->group ] = get_transient( "{$this->group}-pool-map" ) ?: [];
self::$pool_indexes[ $this->group ] = get_transient( "{$this->group}-pool-index" ) ?: -1;
}

/**
* Persist the pool meta information.
*/
private function persist_pool_meta() {
set_transient( "{$this->group}-pool-map", self::$pool_maps[ $this->group ] );
set_transient( "{$this->group}-pool-index", self::$pool_indexes[ $this->group ] );
}

/**
* Advance the index into the pool cache.
*/
private function advance_pool_index() {
self::$pool_indexes[ $this->group ]++;

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

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

}
}
}
Loading