WordPress.org

Make WordPress Core

Opened 16 months ago

Last modified 2 weeks ago

#50567 assigned defect (bug)

Set $post filter in update_post_cache()

Reported by: Cybr Owned by: hellofromTonya
Milestone: 5.9 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 16 months ago.
Sanitize post before storing it in cache.
cg.zip (2.8 MB) - added by Cybr 15 months ago.
Cachegrind outputs
50567.png (196.2 KB) - added by Cybr 15 months ago.

Change History (20)

@Cybr
16 months ago

Sanitize post before storing it in cache.

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


15 months ago

#2 @SergeyBiryukov
15 months ago

  • Milestone changed from Awaiting Review to 5.6

@Cybr
15 months ago

Cachegrind outputs

@Cybr
15 months ago

#3 follow-up: @Cybr
15 months 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 overhead (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.011 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.

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.

Version 0, edited 15 months ago by Cybr (next)

#4 in reply to: ↑ 3 @peterwilsoncc
15 months 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.

#5 @Cybr
14 months ago

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

I looked at the default behavior from the WP_Post class constructor. I don't think another value (like display) would benefit most sites. You can test this, of course. Keep in mind that plugins and themes will (significantly) affect this behavior. We learned to escape as late as possible, so using raw values should be fine for most cases.

Maybe it would be better to call empty( $post->filter ) || $post->filter !== raw.

Yes, I believe that'd be better, too. But, please keep in mind that WP_Post does not check against that, so this change adds a discrepancy. We may want to improve things in the WP_Post class, as well.

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


12 months ago

#7 @hellofromTonya
12 months ago

  • Keywords early added
  • Milestone changed from 5.6 to 5.7

From today's 5.6 core scrub, Sergey:

This could probably use some unit tests. Both #50567 and this one #50568 are nice performance enhancements, but also seem like early tickets, should probably be moved to 5.7.

Moving this ticket to early 5.7 milestone along with #50568.

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

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


10 months ago

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


10 months ago

#10 @hellofromTonya
9 months ago

  • Milestone changed from 5.7 to Future Release

This ticket is marked for early in the alpha cycle. We are now in the Beta cycle and long past early. Moving this ticket and #50568 to Future Release. However, would be great to get both committed early in 5.8 alpha cycle if possible.

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

#11 @Cybr
6 months ago

The patch provided has not caused issues with any installation I manage. It's only two code lines that make WordPress over 60% more performant, let alone the climate impact it could have.

What's holding it back from WP 5.8? Practically, unit tests are already in place.

#12 @peterwilsoncc
6 months ago

  • Milestone changed from Future Release to 5.8

@Cybr I've put this on 5.8, you make an excellent point.

#13 @hellofromTonya
5 months ago

  • Milestone changed from 5.8 to 5.9
  • Owner set to hellofromTonya
  • Status changed from new to assigned

Today is 5.8 Beta 1. Unfortunately ran out of time to do performance checks and code review.

Punting to 5.9 and assigning myself as owner to finish it early and get it committed.

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


8 weeks ago

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


4 weeks ago

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


2 weeks ago

#17 @hellofromTonya
2 weeks ago

  • Keywords early removed

Removing this from early as it is not a requirement for considering this enhancement.

Note: See TracTickets for help on using tickets.