WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 12 months ago

#37114 new enhancement

Allow short-circuiting `get_post_class` for performance

Reported by: bordoni Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.2
Component: Posts, Post Types Keywords: has-patch commit
Focuses: template, performance Cc:

Description

Now that was introduced the usage of custom taxonomies on the get_post_class function we should be able to filter what will happen to the returned classes before we carry on with the logic.

Something just like what we do with the pre_site_option_$option or pre_delete_post.

I am also providing a patch for this in the hopes that we can make this fit on the release 4.6

If I'm missing anything to make this "release-ready" please let me know and I will gladly do it.

Attachments (8)

37114.0.patch (1.2 KB) - added by bordoni 3 years ago.
Patch v0
37114.patch (2.3 KB) - added by sebastian.pisula 3 years ago.
37114.diff (2.7 KB) - added by desrosj 3 years ago.
37144.diff (808 bytes) - added by desrosj 3 years ago.
Patch combining the two scenarios in one filter.
37144.1.patch (1017 bytes) - added by bordoni 3 years ago.
37114.2.patch (976 bytes) - added by desrosj 3 years ago.
Fixing malformed patch. Improved documentation wording.
get_post_class.PNG (19.8 KB) - added by xParham 2 years ago.
37114.2.diff (1.1 KB) - added by desrosj 12 months ago.

Download all attachments as: .zip

Change History (34)

@bordoni
3 years ago

Patch v0

#1 @bordoni
3 years ago

  • Keywords has-patch added

#2 @swissspidy
3 years ago

Where exactly are the performance problems in post_class() in your eyes?

Note that with your patch there would be no escaping of post classes.

#3 @bordoni
3 years ago

Hi @swissspidy, so Taylor explained this really well at this article: https://taylorlovett.com/2016/05/26/post_class-get_post_class-performance-killers-wordpress-woocommerce/

But to summarize it a little bit, it can be taxing depending on the amount of terms you have on a given query for an archive page it can be a performance hit.

On the Escaping, do you think with this kind of Pre Filter we need to apply some escaping? It would be basically duplicating some code from the function it self. But If you think it's a good idea I don't see a problem.

#4 @tlovett1
3 years ago

This would be very helpful.

#5 @peterwilsoncc
3 years ago

I'd like to see a proof of concept for the performance issues before continuing.

I ran a cut down loop on trunk displaying 34 posts on the blog listing page (home) and there was no difference with or without the post_class function. In both cases there were 22 queries on the page.

#6 @tlovett1
3 years ago

It's only noticeable when there are lots of custom taxonomies registered to the post type i.e. WooCommerce.

#7 @bordoni
3 years ago

@peterwilsoncc I've tested with WooCommerce, and it really doesn't seem to slow down if we are dealing with a base query. Unless we use a WP_Query with any of the these two arguments:

<?php
array(
        'update_post_term_cache' => false,
        'fields' => 'ids',
);

Then it would go from 27 queries up to 240ish queries, with only two types of product variation (taxonomies).

Which is not a huge problem bug also not dismissible I think, and the code added here would still follow other WordPress Core functions with the pre_filter.

Last edited 3 years ago by bordoni (previous) (diff)

#8 @sebastian.pisula
3 years ago

I suggest add new filter to block load taxonomies.

#9 @swissspidy
3 years ago

Stumbled upon this again.

On a site with about 30 posts on the front page and post_class() being used for each, get_the_terms() is being called for every post.

That includes the post_format taxonomy, which isn't supported by the theme, but still associated with the post post type. That means for every post there are unneeded SQL queries to get the post format, even though the theme doesn't declare support for it and therefore doesn't care about post formats.

I don't think 37114.patch is the solution here. If someone wants to basically only use half of the get_post_class() output, I'd use 37114.0.patch and add stuff like post type

Last edited 3 years ago by swissspidy (previous) (diff)

#10 @desrosj
3 years ago

I have noticed this issue as well. What if we added a boolean parameter to register_taxonomy() for showing or hiding the classes for that taxonomy. I see this as similar to show_in_nav_menus or show_admin_column. I could whip up a patch if that approach is favorable.

@desrosj
3 years ago

#11 @desrosj
3 years ago

  • Keywords dev-feedback needs-testing added

Attached a refreshed patch. In addition to @sebastian.pisula's filter (which would be a good filter for hiding classes for specific posts or post types), I added a register_taxonomy() parameter (on by default) for disabling classes on a per taxonomy basis.

#12 follow-up: @bordoni
3 years ago

I love the new code, because it kinda does work for the users, but I don't think it's best idea to introduce more fields to registering process for such a specific case.

Keep in mind that all other places in WordPress structure it's used the filter to allow bailing early.

@desrosj
3 years ago

Patch combining the two scenarios in one filter.

#13 in reply to: ↑ 12 @desrosj
3 years ago

Replying to bordoni:

I love the new code, because it kinda does work for the users, but I don't think it's best idea to introduce more fields to registering process for such a specific case.

Keep in mind that all other places in WordPress structure it's used the filter to allow bailing early.

How about the new patch? It combines both use cases into a single filter. Devs can easily remove specific taxonomies from the list, and easily short circuit the foreach loop by returning false or an empty array. Supplying $post allows it to be post specific as well.

#14 @swissspidy
3 years ago

Yeah, a simple filter like in 37144.diff makes the most sense to me at the moment.

#15 @bordoni
3 years ago

Hey I've added one more param to your filter @desrosj and changed a bit of the Docblock, but I like that solution a lot, simple filters FTW.

@bordoni
3 years ago

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


3 years ago

#17 follow-up: @desrosj
3 years ago

@bordoni I'm not sure I like passing the $classes array in the filter. All classes added before this can be determined from the WP_Post object that is passed.

  • Post status: $post->post_status
  • Post type: $post->post_type
  • Post ID: $post->ID
  • Post thumbnail: has_post_thumbnail()
  • Post password required/protected: post_password_required()
  • Sticky: is_sticky()
  • Post format: get_post_format()

This filter would occur before the 'post_class' filter is applied, which is the only way that other classes could be added.

Last edited 3 years ago by desrosj (previous) (diff)

#18 in reply to: ↑ 17 @bordoni
3 years ago

As per what was talked about on #core on Slack: (https://wordpress.slack.com/archives/core/p1475774672004836)

I agree with you there that the classes can be discovered by the WP_Post object, but I was thinking about the classes that are passed initially. E.g.: get_post_class( 'something-fancy' )

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


3 years ago

@desrosj
3 years ago

Fixing malformed patch. Improved documentation wording.

#20 @desrosj
3 years ago

Just as a note, the performance issue here has been solved with improvements to the term cache. However, this is still a nice to have for removing unnecessary classes for a taxonomy that has nothing to do with any front end functionality (CSS or JS).

#21 @xParham
2 years ago

Guys, see the attached file above. This is still a performance killer when there are lots of custom taxonomies. I was able to use a custom function for the theme to fix it for the end user, but cannot do anything for the posts list in the admin dashboard.

#22 @swissspidy
12 months ago

#44792 was marked as a duplicate.

#23 @steveo2000
12 months ago

Still a performance killer on my site with hundreds of taxonomies. The only way I can see this speeding up my scenario is by filtering get_taxonomies() to be empty.

@desrosj
12 months ago

#24 @desrosj
12 months ago

  • Version changed from 4.6 to 4.2

37114.2.diff refreshes the filter and makes the passed arguments more consistent with the post_class filter at the end of get_post_class().

While I think this is a worthwhile filter to add because it allows unnecessary classes to be removed from the markup, neither of the solutions discussed so far here (short-circuiting the function vs. allowing the taxonomies to be removed with a filter) solve the underlying performance issue.

In my test setup, I have 300 posts displaying on the home page with 20 custom taxonomies registered (but I was able to experience the performance hit with as few as 30 posts displaying at once). Each post has random terms from random taxonomies assigned. The performance issue is being caused by the post term cache being primed in the main query on the page. This function consolidates individual queries for each public taxonomy attached to each post into one query for all posts being displayed on the page, reducing the number of queries from several thousand on my test setup to one.

This large query can be prevented with the following code snippet:

function prevent_term_cache_prime( $query ) {
    if ( ! $query->is_main_query() ) {
        return;
    }
    
    $query->set( 'update_post_term_cache', false );
}

However, the tradeoff with the snippet above is the number of queries increases exponentially. In my testing, the current behavior is actually much faster than using the code snippet above, but I do not have any caching enabled. With something like Memcached enabled, it may help prevent running all of those queries at once.

Wondering if @boonebgorges would have any ideas for fixing this.

Also, for reference, this was introduced in r31271.

#25 @boonebgorges
12 months ago

However, the tradeoff with the snippet above is the number of queries increases exponentially. In my testing, the current behavior is actually much faster than using the code snippet above, but I do not have any caching enabled. With something like Memcached enabled, it may help prevent running all of those queries at once.

On my setup, disabling post-term cache priming has dramatically *negative* performance effects, and this is without any persistent caching. So I think that the situation depends heavily on the specifics of the situation. On my setup, I can mitigate the performance issue by forcing update_object_term_cache() inside of get_post_class() even when post term caching has been disabled at the WP_Query level, but seems like a bad solution as well.

Aside from simply rolling back [31271], I can't think of a good all-purpose fix for the problem. So the filter suggested in 37114.2.diff seems as good a solution as any.

#26 @desrosj
12 months ago

  • Keywords commit added; dev-feedback needs-testing removed

Unless a better solution is proposed, I am going to recommend that 37114.2.diff is committed allowing classes for terms assigned to a post in specific taxonomies to be excluded. While this does not directly solve the performance issue caused by priming the post term cache when a large number of taxonomies are assigned to the posts being displayed, it gives developers a little more flexibility.

A combination of removing taxonomies from displaying in post class attributes, disabling post term cache priming, and managing the public/private property of taxonomies, etc. can be used to tune performance. [31271] is a benefit for a large majority of scenarios for most installs, so I don't think that should be reverted.

Note: See TracTickets for help on using tickets.