WordPress.org

Make WordPress Core

Opened 6 weeks ago

Last modified 2 weeks ago

#50567 new defect (bug)

Set $post filter in update_post_cache()

Reported by: Cybr Owned by:
Milestone: 5.6 Priority: normal
Severity: normal Version: 5.5
Component: Posts, Post Types Keywords: has-patch needs-unit-tests
Focuses: performance Cc:

Description

update_post_cache() stores each post provided in the cache, the same cache WP_Post::get_instance() uses. But in doing so, it doesn't necessarily add a $filter to the post.

Because of this, WP_Post::get_instance() will always re-sanitize the post whenever a post stored prior via update_post_cache() is requested--since WP_Post::get_instance() tests for empty( $_post->filter ) after retrieving the post back from cache (but doesn't repopulate the post cache).

This re-sanitization most prominently happens at wp-admin/edit.php, where each post displayed goes through this process about 24 times (on a clean WP installation).

To address this issue, we should prime the post cache with a $filter. The file attached will already improve the aforementioned page's load time by 62%--it's quite substantial.

Attachments (3)

post.php.patch (379 bytes) - added by Cybr 6 weeks ago.
Sanitize post before storing it in cache.
cg.zip (2.8 MB) - added by Cybr 2 weeks ago.
Cachegrind outputs
50567.png (196.2 KB) - added by Cybr 2 weeks ago.

Change History (7)

@Cybr
6 weeks ago

Sanitize post before storing it in cache.

This ticket was mentioned in Slack in #core by cybr. View the logs.


2 weeks ago

#2 @SergeyBiryukov
2 weeks ago

  • Milestone changed from Awaiting Review to 5.6

@Cybr
2 weeks ago

Cachegrind outputs

@Cybr
2 weeks ago

#3 follow-up: @Cybr
2 weeks ago

@peterwilsoncc asked me in Slack if I could post fuller screenshots (50567.png) pertaining to this ticket.

Attached (cg.zip) are before and after CacheGrind outputs generated by xDebug on PHP 7.4.7 via XAMPP on a VM with generous headroom (16GB/4x3.8GHz).

I've used Twenty Twenty with no plugins active on WP 5.4.2 en_US, displaying 20 posts (duplicated default post "Hello World!") at /wp-admin/edit.php, and 10 posts at / (homepage).

To learn how to inspect the files, see https://xdebug.org/docs/profiler. I used QCacheGrind to generate the attached screenshot.

1 unit of Incl./Self/Time is equal to 1 microsecond (0.001ms).

The attached screenshot shows the grind's output in this order:
Home Before / Home After
Admin Before / Admin After

https://core.trac.wordpress.org/raw-attachment/ticket/50567/50567.png

At the bottom-left of each window in the screenshots, you can see the Total Time Cost to generate the page. Since we can't use opcode caching during debugging, file inclusion takes up a generous amount of time. So, you can't calculate the relative performance impact of the improvements to the total page generation time. Nevertheless, you can measure a significant boost there, too.

sanitize_post_field Called Microtime Seconds Improvement
Location Before After Before After Before After =-1*((Af-Be)/Be)
(logged out) / 4 554 276 100 911 6 110 0.101 0.006 94%
(logged in) /wp-admin/edit.php 12 443 483 272 566 10 372 0.273 0.010 96%

So, we can assume almost up to 0.3s improved response times using PHP 7.4, implying a total of 62% of the total page load time (which I tested with OpCache enabled elsewhere).

You'll notice from the same CacheGrind files, sanitize_term_field() is also affected by this issue, where category ID 1 is resanitized over and over again. To accommodate for that, see #50568.

Last edited 2 weeks ago by Cybr (previous) (diff)

#4 in reply to: ↑ 3 @peterwilsoncc
2 weeks ago

  • Keywords has-patch needs-unit-tests added

Replying to Cybr:

Attached (cg.zip) are before and after CacheGrind outputs generated by xDebug on PHP 7.4.7 via XAMPP on a VM with generous headroom (16GB/4x3.8GHz).

I've used Twenty Twenty with no plugins active on WP 5.4.2 en_US, displaying 20 posts (duplicated default post "Hello World!") at /wp-admin/edit.php, and 10 posts at / (homepage).

Thanks for attaching the screen shots and additional summary, it's very much appreciated.

I can see this happening with a very simple test plugin running on the /wp-admin/edit.php screen:

<?php
add_action( 'admin_enqueue_scripts', function() {
        /*
         * Post 72 is off the page (ie, this is a new query).
         * 
         * The wp_cache_get dump includes `filter: raw`
         */
        var_dump( get_post( 72, OBJECT, 'display' ) );
        var_dump( wp_cache_get( 72, 'posts' ) );

        /*
         * Post 38 is on the page and has been returned by WP_Query.
         * 
         * The wp_cache_get dump does not include `filter: raw`
         */
        var_dump( wp_cache_get( 38, 'posts' ) );
        var_dump( get_post( 38 ) );
        exit;
} );

The one question I have about the patch is is there a situation in which $post->filter can be a value other than raw?

If the cache is expected to have raw data, then it would be dandy to be certain that the data is indeed stored that way. Maybe it would be better to call empty( $post->filter ) || $post->filter !== raw.

Note: See TracTickets for help on using tickets.