Allow short-circuiting `get_post_class` for performance

[bordoni]

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.

[bordoni]

Patch v0

[swissspidy]

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.

[bordoni]

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.

[tlovett1]

This would be very helpful.

[peterwilsoncc]

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.

[tlovett1]

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

[bordoni]

@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.

[sebastian.pisula]

I suggest add new filter to block load taxonomies.

[swissspidy]

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

[desrosj]

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]

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.

[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.

[desrosj]

Patch combining the two scenarios in one filter.

[desrosj]

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.

[swissspidy]

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

[bordoni]

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.

[desrosj]

@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.

[bordoni]

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' )

[desrosj]

Fixing malformed patch. Improved documentation wording.

[desrosj]

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).

[xParham]

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.

[swissspidy]

#44792 was marked as a duplicate.

[steveo2000]

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]

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.

[boonebgorges]

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.

[desrosj]

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.

[invelity]

Having 800 terms in a taxonomy, removing the taxonomy loop sped up listing of 20 posts by 68% in our scenario. Having a filter available would solve the issue for us.

[davidbaumwald]

Most recent patch fails against trunk. Marking for a refresh.

[system909]

update patch for trunk

The PR refresh the final patch 37114.3.patch also update the docblock for $classes and $class similar to post_class filter.

Trac ticket: https://core.trac.wordpress.org/ticket/37114

[mukesh27]

@davidbaumwald PR ready for commit.

@peterwilsoncc Can you please re-review the PR?

[mukesh27]

Thank you, Peter, for the review and approving the changes.

@davidbaumwald Can you please review it when you get time?

[peterwilsoncc]

In 54066:

Posts, Post Types: Allow bypassing of term classes in get_post_class().

Introduces the filter wp_post_class_taxonomies to allow developers to bypass the generation of term based classes with the get_post_class() function.

Developers can use this filter to reduce the number of taxonomies for which classes term classes are generated. This can improve performance on sites with a large number of custom taxonomies.

Props boonebgorges, bordoni, davidbaumwald, desrosj, invelity, mukesh27, sebastianpisula, steveo2000, swissspidy, system909, tlovett1, xparham.
Fixes #37114.

Merged in https://core.trac.wordpress.org/changeset/54066 / 37b2a7fb14df0911836e63960e0ad263232aa82e

[SergeyBiryukov]

It looks like the wp_ prefix was ​added for better namespacing:

-	$taxonomies = apply_filters( 'post_class_taxonomies', $taxonomies, $classes, $class, $post->ID );
+	$taxonomies = apply_filters( 'wp_post_class_taxonomies', $taxonomies, $classes, $class, $post->ID );

namespace all the things: Core hasn't been great at this but it's nice to do for new items.

I see the point and I would agree in most cases, especially for filters in newer functions:

• wp_post_revision_title_expanded

or when the function already has a wp_ prefix:

• wp_link_pages_args
• wp_list_pages_excludes
• wp_page_menu_args
• wp_get_attachment_link

However, I think it is also important to take into consideration consistency with the existing filters:

• post_class
• body_class
• comment_class

It has been a long-time practice in core for hook names to start with the function name. There might be occasional exceptions to this, e.g. when the get_ prefix is removed, which is what we see here, but that still matches the functions that are just wrappers of the respective get_*() versions:

• post_class() ← get_post_class()
• body_class() ← get_body_class()
• comment_class() ← get_comment_class()

If we start deviating from this, I believe it would add more inconsistencies to core, and make it harder to remember which hooks have the wp_ prefix and which don't without looking each time in the developer reference. Since we are bound by the backward compatibility promise, it's not always feasible or worth deprecating the older hooks just for better namespacing, and I think having a mix of wp_* and non-wp_* hooks without a predictable pattern is not ideal.

So I would suggest using the post_class_taxonomies name to match post_class. Curious to see what others think.

[mukesh27]

Thanks Sergey for the details information for the function. 37114.3.diff​ LGTM.

[costdev]

@SergeyBiryukov I agree, consistency makes sense here.

[robinwpdeveloper]

Agreed with @SergeyBiryukov .
37114.3.diff LGTM as well.

[davidbaumwald]

In 54208:

Posts, Post Types: Update new wp_post_class_taxonomies filter name for consistency.

In [54066], a new filter was added to alter the taxonomies for which class names are generated for a given post type. At the time, the filter name was prefixed with wp_.

For consistency with filters of a similar type, this change updates the filter name to post_class_taxonomies.

Follow-up to [54066].

Props SergeyBiryukov.
See #37114.

[davidbaumwald]

With the follow-up commit, that should close this ticket.

The new filter does deserve a small call-out on the Misc Dev note.