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.

Sign up for GitHub

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

Jump to bottom

Improve post processor response cache #1325

Merged
merged 26 commits into from
Aug 23, 2018
Merged

Improve post processor response cache #1325

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
0a5e3f2
Add post-processor cache effectiveness.
Aug 7, 2018
fe8bf17
Turn off response caching when exceeding cache miss threshold.
Aug 7, 2018
1d1f399
Expose keys and add tests to validate caching conditions.
Aug 7, 2018
7e6f874
Convert response cache key to internal variable instead of public sta…
Aug 9, 2018
eab2d95
Convert post-processor cache key to internal variable instead of publ…
Aug 9, 2018
d7afb0d
Init $caches_for_url to an empty array.
Aug 9, 2018
73e72b9
Set post-processor cache time to 10 minutes.
Aug 9, 2018
7316ee0
Convert post-processor cache to occurrence instead of page-by-page.
Aug 9, 2018
773bfee
Store the cache miss URL in an option. Then disable.
Aug 10, 2018
75611f5
Add notice to AMP general screen.
Aug 10, 2018
bb39b89
Add caching section to the AMP General sub page.
Aug 13, 2018
2c760bd
Merge branch 'develop' into improve/post-processor-response-cache
Aug 13, 2018
efd9d93
Automatically disable response caching when threshold is exceeded.
Aug 13, 2018
f37e7ff
Handle re-enabling the response cache.
Aug 13, 2018
b0dc43d
Top level notice is dismissible.
Aug 13, 2018
8e553e9
Improve top level notice to be user friendly and link to Wiki.
Aug 13, 2018
1e6a8af
Improve field description and inline warning.
Aug 13, 2018
b05a38f
Merge branch 'develop' into improve/post-processor-response-cache
Aug 22, 2018
0274715
Call AMP_Theme_Support::reset_cache_miss_url_option() directly.
Aug 22, 2018
ecb1587
Check wp_using_ext_object_cache() as part of setting and enable.
Aug 22, 2018
752de78
Use wp_using_ext_object_cache() as the default value of 'enable_respo…
Aug 22, 2018
e575cde
Show notice only when the cache has been disabled due to exceeding th…
Aug 23, 2018
f6716ec
Add description for the enable checkbox.
Aug 23, 2018
bf3dd20
Fix self reference for local constant.
Aug 23, 2018
cdd0506
Rename response cache to post-processor cache
westonruter Aug 23, 2018
65ad9fa
Bump CACHE_MISS_THRESHOLD from 3 to 20
westonruter Aug 23, 2018
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
67 changes: 62 additions & 5 deletions includes/class-amp-theme-support.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,20 @@ class AMP_Theme_Support {
*/
const RESPONSE_CACHE_GROUP = 'amp-response';

/**
* Post-processor cache group name.
*
* @var string
*/
const POST_PROCESSOR_CACHE_EFFECTIVENESS = 'post_processor_cache_effectiveness';

/**
* Cache miss threshold for determining when to disable post-processor cache.
*
* @var int
*/
const CACHE_MISS_THRESHOLD = 3;

/**
* Sanitizer classes.
*
Expand Down Expand Up @@ -98,6 +112,20 @@ class AMP_Theme_Support {
*/
protected static $support_added_via_option = false;

/**
* Response cache key.
*
* @var string
*/
public static $response_cache_key;
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary? Aren't references limited to a single method? Since this is a static class it would be better of we limited the state that is persisted in it's class variables IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@westonruter I added this public property for the following purposes:

  1. to give us the ability to deeply test by knowing what the cache key is and then checking for it in the cache.
  2. to simplify access to the logic within the caching closure, i.e. meaning that we don't have to pass it via use.

It serves no other external purpose as part of an API.

Let's think about it.

From a testing point of view, we can infer that we disabled the cache by the state of the post-processor cache. While that's not a direct test on the response cache, both are within the closure. Therefore, it's a minimal risk to test by inference.

Hmm, I like that approach better actually. We get the benefit only exposing what is needed externally while avoided the extra work of resetting persistent state.

I'll make that change.


/**
* Post-processor cache key.
*
* @var string
*/
public static $post_processor_cache_key;

/**
* Initialize.
*
Expand Down Expand Up @@ -1760,19 +1788,36 @@ public static function prepare_response( $response, $args = array() ) {
$current_url = amp_get_current_url();
$ampless_url = amp_remove_endpoint( $current_url );

// When response caching is enabled, determine if it should be turned off for cache misses.
$caches_for_url = null;
self::$post_processor_cache_key = null;
if ( true === $args['enable_response_caching'] ) {
self::$post_processor_cache_key = md5( $current_url );
$caches_for_url = wp_cache_get( self::$post_processor_cache_key, self::POST_PROCESSOR_CACHE_EFFECTIVENESS );
Copy link
Member

Choose a reason for hiding this comment

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

Sone of the subsequent checks could perhaps be simplified by checking if this is an array here, and if not, just set it to be an empty array.


$args['enable_response_caching'] = (
empty( $caches_for_url )
||
! is_array( $caches_for_url )
||
count( $caches_for_url ) < self::CACHE_MISS_THRESHOLD
);
}

// Return cache if enabled and found.
$cache_response = null;
$cache_response = null;
self::$response_cache_key = '';
if ( true === $args['enable_response_caching'] ) {
// Set response cache hash, the data values dictates whether a new hash key should be generated or not.
$response_cache_key = md5( wp_json_encode( array(
self::$response_cache_key = md5( wp_json_encode( array(
$args,
$response,
self::$sanitizer_classes,
self::$embed_handlers,
AMP__VERSION,
) ) );

$response_cache = wp_cache_get( $response_cache_key, self::RESPONSE_CACHE_GROUP );
$response_cache = wp_cache_get( self::$response_cache_key, self::RESPONSE_CACHE_GROUP );

// Make sure that all of the validation errors should be sanitized in the same way; if not, then the cached body should be discarded.
$blocking_error_count = 0;
Expand Down Expand Up @@ -1802,9 +1847,21 @@ public static function prepare_response( $response, $args = array() ) {
return $response_cache['body'];
}

$cache_response = function( $body, $validation_results ) use ( $response_cache_key ) {
$cache_response = function( $body, $validation_results ) use ( $caches_for_url ) {
if ( empty( $caches_for_url ) ) {
$caches_for_url = array( AMP_Theme_Support::$response_cache_key );
} else {
$caches_for_url[] = AMP_Theme_Support::$response_cache_key;
Copy link
Member

Choose a reason for hiding this comment

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

Per above, this could be the sole line instead of the if/else.

}
wp_cache_set(
AMP_Theme_Support::$post_processor_cache_key,
$caches_for_url,
AMP_Theme_Support::POST_PROCESSOR_CACHE_EFFECTIVENESS,
MONTH_IN_SECONDS
);

return wp_cache_set(
$response_cache_key,
AMP_Theme_Support::$response_cache_key,
compact( 'body', 'validation_results' ),
AMP_Theme_Support::RESPONSE_CACHE_GROUP,
MONTH_IN_SECONDS
Expand Down
246 changes: 152 additions & 94 deletions tests/test-class-amp-theme-support.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1423,85 +1423,7 @@ public function test_filter_customize_partial_render() {
*/
public function test_prepare_response() {
// phpcs:disable WordPress.WP.EnqueuedResources.NonEnqueuedScript, WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet
add_filter( 'amp_validation_error_sanitized', '__return_true' );
global $wp_widget_factory, $wp_scripts, $wp_styles;
$wp_scripts = null;
$wp_styles = null;

add_theme_support( 'amp' );
AMP_Theme_Support::init();
AMP_Theme_Support::finish_init();
$wp_widget_factory = new WP_Widget_Factory();
wp_widgets_init();

$this->assertTrue( is_amp_endpoint() );

add_action( 'wp_enqueue_scripts', function() {
wp_enqueue_script( 'amp-list' );
} );
add_action( 'wp_print_scripts', function() {
echo '<!-- wp_print_scripts -->';
} );

add_action( 'wp_print_styles', function() {
echo '<link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Tangerine">';
} );

add_filter( 'script_loader_tag', function( $tag, $handle ) {
if ( ! wp_scripts()->get_data( $handle, 'conditional' ) ) {
$tag = preg_replace( '/(?<=<script)/', " handle='$handle' ", $tag );
}
return $tag;
}, 10, 2 );

add_action( 'wp_footer', function() {
wp_print_scripts( 'amp-mathml' );
?>
<amp-mathml layout="container" data-formula="\[x = {-b \pm \sqrt{b^2-4ac} \over 2a}.\]"></amp-mathml>
<?php
}, 1 );

add_filter( 'get_site_icon_url', function() {
return home_url( '/favicon.png' );
} );

ob_start();
?>
<!DOCTYPE html>
<html amp>
<head>
<?php wp_head(); ?>
<script data-head>document.write('Illegal');</script>
<script async custom-element="amp-dynamic-css-classes" src="https://cdn.ampproject.org/v0/amp-dynamic-css-classes-0.1.js"></script>
</head>
<body><!-- </body></html> -->
<img width="100" height="100" src="https://example.com/test.png">
<audio width="400" height="300" src="https://example.com/audios/myaudio.mp3"></audio>
<amp-ad type="a9"
width="300"
height="250"
data-aax_size="300x250"
data-aax_pubname="test123"
data-aax_src="302"></amp-ad>

<?php wp_footer(); ?>

<button onclick="alert('Illegal');">no-onclick</button>

<style>body { background: black; }</style>

<amp-experiment>
<script type="application/json">
{ "aExperiment": {} }
</script>
</amp-experiment>
</body>
</html>
<!--comment-after-html-->
<div id="after-html"></div>
<!--comment-end-html-->
<?php
$original_html = trim( ob_get_clean() );
$original_html = $this->get_original_html();
$sanitized_html = AMP_Theme_Support::prepare_response( $original_html );

$this->assertNotContains( 'handle=', $sanitized_html );
Expand Down Expand Up @@ -1596,47 +1518,183 @@ public function test_prepare_response() {
return AMP_Theme_Support::prepare_response( $original_html, $prepare_response_args );
};

$get_server_timing_header_count = function() {
return count( array_filter(
AMP_Response_Headers::$headers_sent,
function( $header ) {
return 'Server-Timing' === $header['name'];
}
) );
};

// Test that first response isn't cached.
$first_response = $call_prepare_response();
$this->assertGreaterThan( 0, $get_server_timing_header_count() );
$this->assertGreaterThan( 0, $this->get_server_timing_header_count() );
$this->assertContains( '<html amp>', $first_response ); // Note: AMP because sanitized validation errors.
$this->reset_post_processor_cache_effectiveness();

// Test that response cache is return upon second call.
$this->assertEquals( $first_response, $call_prepare_response() );
$this->assertSame( 0, $get_server_timing_header_count() );
$this->assertSame( 0, $this->get_server_timing_header_count() );
$this->reset_post_processor_cache_effectiveness();

// Test new cache upon argument change.
$prepare_response_args['test_reset_by_arg'] = true;
$call_prepare_response();
$this->assertGreaterThan( 0, $get_server_timing_header_count() );
$this->assertGreaterThan( 0, $this->get_server_timing_header_count() );
$this->reset_post_processor_cache_effectiveness();

// Test response is cached.
$call_prepare_response();
$this->assertSame( 0, $get_server_timing_header_count() );
$this->assertSame( 0, $this->get_server_timing_header_count() );
$this->reset_post_processor_cache_effectiveness();

// Test that response is no longer cached due to a change whether validation errors are sanitized.
remove_filter( 'amp_validation_error_sanitized', '__return_true' );
add_filter( 'amp_validation_error_sanitized', '__return_false' );
$prepared_html = $call_prepare_response();
$this->assertGreaterThan( 0, $get_server_timing_header_count() );
$this->assertGreaterThan( 0, $this->get_server_timing_header_count() );
$this->assertContains( '<html>', $prepared_html ); // Note: no AMP because unsanitized validation error.
$this->reset_post_processor_cache_effectiveness();

// And test response is cached.
$call_prepare_response();
$this->assertSame( 0, $get_server_timing_header_count() );
$this->assertSame( 0, $this->get_server_timing_header_count() );

// phpcs:enable WordPress.WP.EnqueuedResources.NonEnqueuedScript, WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet
}

/**
* Test post-processor cache effectiveness in AMP_Theme_Support::prepare_response().
*/
public function test_post_processor_cache_effectiveness() {
$original_html = $this->get_original_html();
$args = array( 'enable_response_caching' => true );
$this->reset_post_processor_cache_effectiveness();

// Test the response is not cached after exceeding the cache miss threshold.
for ( $num_calls = 1, $max = AMP_Theme_Support::CACHE_MISS_THRESHOLD + 1; $num_calls <= $max; $num_calls++ ) {
// Simulate dynamic changes in the content.
$original_html = str_replace( 'dynamic-id-', "dynamic-id-{$num_calls}-", $original_html );

AMP_Response_Headers::$headers_sent = array();
AMP_Validation_Manager::$validation_results = array();
AMP_Theme_Support::prepare_response( $original_html, $args );

$caches_for_url = wp_cache_get( AMP_Theme_Support::$post_processor_cache_key, AMP_Theme_Support::POST_PROCESSOR_CACHE_EFFECTIVENESS );

// When we've met the threshold, check that caching did not happen.
if ( $num_calls > AMP_Theme_Support::CACHE_MISS_THRESHOLD ) {
$this->assertEquals( $num_calls - 1, count( $caches_for_url ) );
$this->assertEmpty( AMP_Theme_Support::$response_cache_key );
} else {
$this->assertEquals( $num_calls, count( $caches_for_url ) );
$this->assertNotEmpty( AMP_Theme_Support::$response_cache_key );
$this->assertNotEmpty( wp_cache_get( AMP_Theme_Support::$response_cache_key, AMP_Theme_Support::RESPONSE_CACHE_GROUP ) );
}

$this->assertGreaterThan( 0, $this->get_server_timing_header_count() );
}
}

/**
* Initializes and returns the original HTML.
*/
private function get_original_html() {
// phpcs:disable WordPress.WP.EnqueuedResources.NonEnqueuedScript, WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet
add_filter( 'amp_validation_error_sanitized', '__return_true' );
global $wp_widget_factory, $wp_scripts, $wp_styles;
$wp_scripts = null;
$wp_styles = null;

add_theme_support( 'amp' );
AMP_Theme_Support::init();
AMP_Theme_Support::finish_init();
$wp_widget_factory = new WP_Widget_Factory();
wp_widgets_init();

$this->assertTrue( is_amp_endpoint() );

add_action( 'wp_enqueue_scripts', function() {
wp_enqueue_script( 'amp-list' );
} );
add_action( 'wp_print_scripts', function() {
echo '<!-- wp_print_scripts -->';
} );

add_action( 'wp_print_styles', function() {
echo '<link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Tangerine">';
} );

add_filter( 'script_loader_tag', function( $tag, $handle ) {
if ( ! wp_scripts()->get_data( $handle, 'conditional' ) ) {
$tag = preg_replace( '/(?<=<script)/', " handle='$handle' ", $tag );
}
return $tag;
}, 10, 2 );

add_action( 'wp_footer', function() {
wp_print_scripts( 'amp-mathml' );
?>
<amp-mathml layout="container" data-formula="\[x = {-b \pm \sqrt{b^2-4ac} \over 2a}.\]"></amp-mathml>
<?php
}, 1 );

add_filter( 'get_site_icon_url', function() {
return home_url( '/favicon.png' );
} );

ob_start();
?>
<!DOCTYPE html>
<html amp>
<head>
<?php wp_head(); ?>
<script data-head>document.write('Illegal');</script>
<script async custom-element="amp-dynamic-css-classes" src="https://cdn.ampproject.org/v0/amp-dynamic-css-classes-0.1.js"></script>
</head>
<body><!-- </body></html> -->
<div id="dynamic-id-0"></div>
<img width="100" height="100" src="https://example.com/test.png">
<audio width="400" height="300" src="https://example.com/audios/myaudio.mp3"></audio>
<amp-ad type="a9"
width="300"
height="250"
data-aax_size="300x250"
data-aax_pubname="test123"
data-aax_src="302"></amp-ad>

<?php wp_footer(); ?>

<button onclick="alert('Illegal');">no-onclick</button>

<style>body { background: black; }</style>

<amp-experiment>
<script type="application/json">
{ "aExperiment": {} }
</script>
</amp-experiment>
</body>
</html>
<!--comment-after-html-->
<div id="after-html"></div>
<!--comment-end-html-->
<?php
return trim( ob_get_clean() );
// phpcs:enable WordPress.WP.EnqueuedResources.NonEnqueuedScript, WordPress.WP.EnqueuedResources.NonEnqueuedStylesheet
}

/**
* Returns the "Server-Timing" header count.
*/
private function get_server_timing_header_count() {
return count( array_filter(
AMP_Response_Headers::$headers_sent,
function( $header ) {
return 'Server-Timing' === $header['name'];
}
) );
}

/**
* Reset cached URLs in post-processor cache effectiveness.
*/
private function reset_post_processor_cache_effectiveness() {
wp_cache_delete( md5( amp_get_current_url() ), AMP_Theme_Support::POST_PROCESSOR_CACHE_EFFECTIVENESS );
}

/**
* Test prepare_response for bad/non-HTML.
*
Expand Down