Make WordPress Core

Opened 7 years ago

Closed 12 months ago

#37114 closed enhancement (fixed)

Allow short-circuiting `get_post_class` for performance

Reported by: bordoni's profile bordoni Owned by: davidbaumwald's profile davidbaumwald
Milestone: 6.1 Priority: normal
Severity: normal Version: 4.2
Component: Posts, Post Types Keywords: has-patch commit needs-dev-note
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 (10)

37114.0.patch (1.2 KB) - added by bordoni 7 years ago.
Patch v0
37114.patch (2.3 KB) - added by sebastian.pisula 7 years ago.
37114.diff (2.7 KB) - added by desrosj 7 years ago.
37144.diff (808 bytes) - added by desrosj 7 years ago.
Patch combining the two scenarios in one filter.
37144.1.patch (1017 bytes) - added by bordoni 7 years ago.
37114.2.patch (976 bytes) - added by desrosj 7 years ago.
Fixing malformed patch. Improved documentation wording.
get_post_class.PNG (19.8 KB) - added by xParham 6 years ago.
37114.2.diff (1.1 KB) - added by desrosj 5 years ago.
37114.3.patch (1.2 KB) - added by system909 14 months ago.
update patch for trunk
37114.3.diff (716 bytes) - added by SergeyBiryukov 13 months ago.

Download all attachments as: .zip

Change History (52)

@bordoni
7 years ago

Patch v0

#1 @bordoni
7 years ago

  • Keywords has-patch added

#2 @swissspidy
7 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
7 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
7 years ago

This would be very helpful.

#5 @peterwilsoncc
7 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
7 years ago

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

#7 @bordoni
7 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 7 years ago by bordoni (previous) (diff)

#8 @sebastian.pisula
7 years ago

I suggest add new filter to block load taxonomies.

#9 @swissspidy
7 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 7 years ago by swissspidy (previous) (diff)

#10 @desrosj
7 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
7 years ago

#11 @desrosj
7 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
7 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
7 years ago

Patch combining the two scenarios in one filter.

#13 in reply to: ↑ 12 @desrosj
7 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
7 years ago

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

#15 @bordoni
7 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
7 years ago

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


7 years ago

#17 follow-up: @desrosj
7 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 7 years ago by desrosj (previous) (diff)

#18 in reply to: ↑ 17 @bordoni
7 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.


7 years ago

@desrosj
7 years ago

Fixing malformed patch. Improved documentation wording.

#20 @desrosj
7 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
6 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
5 years ago

#44792 was marked as a duplicate.

#23 @steveo2000
5 years 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
5 years ago

#24 @desrosj
5 years 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
5 years 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
5 years 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.

#27 @invelity
4 years ago

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.

#28 @davidbaumwald
3 years ago

  • Keywords needs-refresh added; commit removed

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

@system909
14 months ago

update patch for trunk

#29 @davidbaumwald
14 months ago

  • Owner set to davidbaumwald
  • Status changed from new to reviewing

#30 @davidbaumwald
14 months ago

  • Milestone changed from Awaiting Review to 6.1

This ticket was mentioned in PR #3092 on WordPress/wordpress-develop by mukeshpanchal27.


13 months ago
#31

  • Keywords needs-refresh removed

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

#32 @mukesh27
13 months ago

  • Keywords commit added

@davidbaumwald PR ready for commit.

mukeshpanchal27 commented on PR #3092:


13 months ago
#33

@peterwilsoncc Can you please re-review the PR?

#34 @mukesh27
13 months ago

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

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

#35 @peterwilsoncc
13 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

peterwilsoncc commented on PR #3092:


13 months ago
#36

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

#37 @SergeyBiryukov
13 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#38 @mukesh27
13 months ago

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

#39 @costdev
13 months ago

@SergeyBiryukov I agree, consistency makes sense here.

#40 @robinwpdeveloper
13 months ago

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

#41 @davidbaumwald
12 months ago

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.

#42 @davidbaumwald
12 months ago

  • Keywords needs-dev-note added
  • Resolution set to fixed
  • Status changed from reopened to closed

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

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

Note: See TracTickets for help on using tickets.