WordPress.org

Make WordPress Core

Opened 17 months ago

Last modified 3 weeks ago

#50567 assigned defect (bug)

Set $post filter in update_post_cache()

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

Change History (29)

@Cybr
17 months ago

Sanitize post before storing it in cache.

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


16 months ago

#2 @SergeyBiryukov
16 months ago

  • Milestone changed from Awaiting Review to 5.6

@Cybr
16 months ago

Cachegrind outputs

@Cybr
16 months ago

#3 follow-up: @Cybr
16 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 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 16 months ago by Cybr (previous) (diff)

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


13 months ago

#7 @hellofromTonya
13 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.


12 months ago

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


11 months ago

#10 @hellofromTonya
10 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
7 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
7 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
6 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.


3 months ago

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


2 months ago

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


2 months ago

#17 @hellofromTonya
2 months ago

  • Keywords early removed

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

#18 @peterwilsoncc
4 weeks ago

I've just caught up on this, I'd like to get it in pre feature freeze.

I still think the cached value will need to be raw sanitization as that's the most common use case (almost all calls to get post are simply get_post( $post_id )).

@Cybr would you mind creating a PR on github.com/wordpress/wordpress-develop? That will allow @hellofromTonya and I to collaborate on the PR, whereas if either of us create one you will be unable to push to it.

#20 @Cybr
4 weeks ago

I still think the cached value will need to be raw sanitization as that's the most common use case (almost all calls to get post are simply get_post( $post_id )).

This skipped my mind, so I forwarded the PR as-is-proposed in the patch attached to this ticket.

Still, another check against 'raw' !== $post->filter wouldn't hurt. Please note the discrepancy with WP_Post::get_instance(), which could be addressed as described below.

This...:

<?php
// ..
} elseif ( empty( $_post->filter ) ) {
        $_post = sanitize_post( $_post, 'raw' );
}
// ..

...would then become:

<?php
// ..
} elseif ( empty( $_post->filter ) || 'raw' !== $_post->filter ) {
        $_post = sanitize_post( $_post, 'raw' );
}
// ..

@peterwilsoncc, do you wish for me to forward that in the PR?

#21 @prbot
4 weeks ago

sybrew commented on PR #1806:

Not being used to PRs (I mostly push to master like a real pro 👀), I didn't know my commit push instantly merged with the PR, forgoing courtesy awaiting a response on my Core ticket reply.

#22 @prbot
4 weeks ago

peterwilsoncc commented on PR #1806:

Not being used to PRs (I mostly push to master like a real pro 👀), I didn't know my commit push instantly merged with the PR, forgoing courtesy awaiting a response on my Core ticket reply.

Don't worry, always feel free to push modifications to your own PRs. You need to be happy with your contributions as much as anyone else. If a committer catches something, they won't be shy making suggestions. As we both know ;)

The logic check to ensure raw sanitization makes sense to me.

#23 @prbot
4 weeks ago

peterwilsoncc commented on PR #1806:

@sybrew Do you have any experience writing unit tests in PHPUnit?

Re-reading the ticket, I noticed it mentioned that it would be good to have some tests in place to make sure committing this code doesn't break anything. I think it's a wise suggestion.

#24 @prbot
3 weeks ago

sybrew commented on PR #1806:

@peterwilsoncc I do not have experience with PHPUnit. However, since the posts/query system is fundamental to WordPress, I believe all necessary query tests exist. It is why I brought it up in comment:11.

A few applicable examples I could find (and all of which passed), are:

  1. Tests_Post_Query::test_the_posts_filter()
  2. Tests_Post_GetPages::test_get_pages_cache() et al. (why didn't this one catch the bug this ticket addresses?)
  3. Everything in Tests_Post_GetPosts.

Is there a scenario you'd like to have explicitly tested, which isn't covered already in many other tests?

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


3 weeks ago

#26 @hellofromTonya
3 weeks ago

  • Milestone changed from 5.9 to 6.0

Today is 5.9 Beta 1 (in a couple of hours potentially). As the PR needs tests and review approval, moving it to 6.0. Adding to my needs unit tests queue and will get it done in 6.0.

Note: See TracTickets for help on using tickets.