[wp-trac] [WordPress Trac] #18631: wp_set_object_terms() should only wp_update_term_count() for affected terms

[WordPress Trac]

#18631: wp_set_object_terms() should only wp_update_term_count() for affected terms
--------------------------+------------------------------------
 Reporter:  jeremyclarke  |      Owner:
     Type:  defect (bug)  |     Status:  new
 Priority:  normal        |  Milestone:  Awaiting Review
Component:  Taxonomy      |    Version:
 Severity:  normal        |   Keywords:  has-patch dev-feedback
--------------------------+------------------------------------
 {{{
 wp_set_object_terms($object_id, $terms, $taxonomy, $append = false)
 }}}

 == The problem ==

 Currently, after creating/inserting any new terms passed in via the
 {{{$terms}}} argument, {{{wp_set_object_terms()}}} will run
 {{{wp_update_term_count()}}} on all of the {{{$terms}}} passed in,
 regardless of whether they were actually affected in any way. This seems
 really wasteful to me, and as far as I can tell causes huge problems when
 trying to delete large terms or remove terms from many posts at once.

 If you look at the delete section of the function you can see that it
 notes the terms that are being deleted, and after deletion runs
 {{{wp_update_term_count()}}} '''only''' on those terms. This is the way it
 should be.

 The main section that adds terms, on the other hand, ignores whether a
 term was affected and updates the counts of all terms passed in. Inside
 the {{{foreach}}} loop it carefully checks if each term is already
 attached to the object, and if it is not, it adds it. Unfortunately, the
 call to {{{wp_update_term_count()}}} that comes next fails to use this
 information, updating all categories both new and old.

 IMHO only posts that weren't already on the object should have their
 counts re-generated and re-saved, as there is no reason to believe that
 the other terms have changed.

 Am I missing something? I can't see a reason to reset counts on the other
 terms that weren't added to the object.

 == Importance ==

 This may seem trivial, but when adding/removing a single category to/from
 hundreds of posts the re-counting time adds up quickly (deleting one term
 took 18 minutes on my site!) In such a scenario only the one term should
 have it's count redone, but every single term on every single post ends up
 being recounted over and over because they are all being passed in. Sure,
 {{{wp_defer_term_counting()}}} is a partial solution to this issue, but
 even there it should only be deferring that one term that is being
 added/deleted, not all the terms that are merely being confirmed as
 already on the post.

 I noticed this because the {{{$append}}} version of
 {{{wp_set_object_terms()}}} is HUGELY faster on the site I'm working on,
 because when you use it you don't pass in already-assigned {{{$terms}}},
 and thus you don't encounter the bug I'm describing.

 Unlike for adding a single category, where you can just pass in one
 {{{$term}}} and {{{$append=true}}}, there is currently no matching system
 for removing a single category, you are locked into passing in all terms
 that were previously on the object but with your deleted term removed (see
 {{{wp_delete_term()}}} for an example). IMHO this is why
 {{{wp_delete_term()}}} has such atrocious performance on large sites with
 lots of terms (see #4365).

 FWIW the issue of simple term-removal will be solved by #15475 , which
 adds a {{{wp_remove_object_terms()}}} function that works as the opposite
 of {{{wp_set_object_terms()}}} with {{{$append=true}}}. If you look at the
 patch on that ticket you'll see that it has the same property as the
 deletion section of wp_set_object_terms: it only resets the term count on
 terms that were actually deleted.

 == Solution ==

 My solution (patch attached) is straightforward: Create a new array in
 {{{wp_set_object_terms()}}} called {{{$new_tt_ids}}} and add tt_ids of
 terms into it as they are added to the object, but not if they were
 already attached to it. Then when running {{{wp_update_term_count()}}} use
 that array instead of the full array of all $tt_ids set on the object.

 After some testing I've noticed huge improvements in performance of
 deleting categories :

 Without patch (# of posts in category/seconds for AJAX to finish deleting
 it)
 * 12 / 21s
 * 86 / 156s

 With patch
 * 12 / .76s
 * 112 / 2.72s
 * 408 / 10.2s

 I also tested my own term migration system which uses
 {{{wp_defer_term_counting()}}} and the difference was significant, though
 not as pronounced for predictable reasons (~7 seconds for 50 posts before
 patch, ~1 second for 50 posts after patch).

 P.S. Is there a reason {{{wp_delete_term()}}} doesn't use
 {{{wp_defer_term_counting()}}} while deleting posts? Seems like it would
 save a lot of trouble!

-- 
Ticket URL: <http://core.trac.wordpress.org/ticket/18631>
WordPress Trac <http://core.trac.wordpress.org/>
WordPress blogging software