Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#32144 closed defect (bug) (fixed)

get_terms with child_of

Reported by: shifty51's profile shifty51 Owned by: boonebgorges's profile boonebgorges
Milestone: 4.2.2 Priority: normal
Severity: normal Version: 4.2
Component: Taxonomy Keywords: fixed-major
Focuses: performance Cc:

Description

Hello, I have a problem with the "get_terms" when I use "child_of" since I installed version 4.2 of wordpress. The error is as follows

Fatal error: Maximum execution time of 30 seconds exceeded in
mywebsite.com/wp-includes/taxonomy.php on line 3958.

The term used in 'child of "a lot of childs (More than 800) and up to 2 levels and is itself a child of another term.
The line in question is in /wp-includes/taxonomy.php this:

// Do not recurse if we've already APPROBATION the term as a child - this Indicates a loop.
if (in_array ($ term-> term_id, $ ancestors)) {
continue;
}

I tried disabling all plugins and activating the twentyfifteen theme, simply by adding taxonomy declaration in the function.php file of the theme:

/ *
 * TAXONOMIES
 * /
add_action ('init', 'create_dossier_taxonomies', 0);

create_dossier_taxonomies function () {
  // Add new taxonomy, make it hierarchical (like categories)
  $ Labels = array (
    'Name' => _x ('Folders', 'general taxonomy name'),
    'Singular_name' => _x ('File', 'taxonomy singular name'),
    'Search_items' => __ ('Search Folder')
    'All_items' => __ ('All files')
    'Parent_item' => __ ('directory')
    'Parent_item_colon' => __ ('Parent Folder:')
    'Edit_item' => __ ('Edit File')
    'Update_item' => __ ('Update folder')
    'Add_new_item' => __ ('Add Folder')
    'New_item_name' => __ ('New folder name')
    'Menu_name' => __ ('File')
  );

  $ Args = array (
    'Hierarchical' => true,
    'Label' => $ labels
    'Show_ui' => true,
    'Show_admin_column' => true,
    'Query_var' => true,
    'Rewrite' => array ('slug' => 'file')
  );

  register_taxonomy ('file', array ('post', 'short'), $ args);
  // Add new taxonomy, make it hierarchical (like categories)

}

When I commented these lines I have no problem. Looking in the version 4.1.3, I feel that the function in question has been rewritten. It would there be a problem with "in_array"?

Sorry for my english...

Attachments (1)

32144.diff (2.0 KB) - added by boonebgorges 8 years ago.

Download all attachments as: .zip

Change History (10)

#1 @boonebgorges
8 years ago

If you're experiencing memory issues with this in_array() check, it suggests that you have a very large number of taxonomy terms, arranged in very deep hierarchies. Is this right? Can you give some numbers here - about how many folders do you have, and are they very very nested?

I can see a change we could make that should improve performance here (switch from in_array() to isset(), and flip the array), but before making this change, I want to be sure that this isn't a symptom of a deeper problem.

#2 follow-up: @shifty51
8 years ago

Ok, first thank you for your reply! The term in question is at level 1 of the taxonomy. It contains more than 800 childs (889).Not very nested. Some of the child have one lower level with a few children (10 at most). I tried to replace in_array by isset. The site no longer crashes but all terms are not apparent.
The structure :
Dossier (taxonomy)
--artiste (the term with 800 childs)
--localisation (less childs, no problem)

Last edited 8 years ago by shifty51 (previous) (diff)

@boonebgorges
8 years ago

#3 in reply to: ↑ 2 ; follow-up: @boonebgorges
8 years ago

Replying to shifty51:

Ok, first thank you for your reply! The term in question is at level 1 of the taxonomy. It contains more than 800 childs (889).Not very nested. Some of the child have one lower level with a few children (10 at most). I tried to replace in_array by isset. The site no longer crashes but all terms are not apparent.
The structure :
Dossier (taxonomy)
--artiste (the term with 800 childs)
--localisation (less childs, no problem)

Thanks for the additional details. When you said you tried to replace in_array by isset, did you try something like 32144.diff? If not, would you mind giving it a spin to see if it solves your problem? If so, I'll try to run some more precise benchmarks.

#4 in reply to: ↑ 3 @shifty51
8 years ago

Replying to boonebgorges:

Replying to shifty51:

Ok, first thank you for your reply! The term in question is at level 1 of the taxonomy. It contains more than 800 childs (889).Not very nested. Some of the child have one lower level with a few children (10 at most). I tried to replace in_array by isset. The site no longer crashes but all terms are not apparent.
The structure :
Dossier (taxonomy)
--artiste (the term with 800 childs)
--localisation (less childs, no problem)

Thanks for the additional details. When you said you tried to replace in_array by isset, did you try something like 32144.diff? If not, would you mind giving it a spin to see if it solves your problem? If so, I'll try to run some more precise benchmarks.

I tried just to change this part of the code.

if ( isset( $ancestors[ $term->term_id ] ) )

With all of your changes, it works!
Thank you! Will it is likely that this change to take effect in future versions?

Last edited 8 years ago by shifty51 (previous) (diff)

#5 @boonebgorges
8 years ago

  • Milestone changed from Awaiting Review to 4.2.2

Thanks, shifty51!

I've run some basic tests, and it looks like the performance benefit of switching to a keyed array + isset are very dramatic. For sites with a large number of terms in a taxonomy hierarchy with three or more terms, the difference can be several orders of magnitude. I created about 6000 terms, made 3000 of them a child of $foo, and then another 1000 a child of one of $foo's children. Then I ran get_terms( array( 'child_of' => $foo, 'hide_empty' => false ) ). Results:

without 32144.diff: 26s
with 32144.diff: ~0.4s

Because this problem has a potential to be pretty big on certain sorts of sites, and because it was introduced in 4.2, I'm moving to 4.2.x consideration.

#6 @boonebgorges
8 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 32326:

Improve performance of loop detection in _get_term_children().

Using an array keyed by term_id allows us to use isset() rather than the
slower in_array(). In addition, it lets us avoid the use of wp_list_pluck()
on large arrays, and helps us to avoid arrays that are unnecessarily large due
to duplicate entries.

Fixes #32144 for trunk.

#7 @boonebgorges
8 years ago

  • Component changed from General to Taxonomy
  • Focuses performance added
  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.2.2.

#8 @SergeyBiryukov
8 years ago

#32245 was marked as a duplicate.

#9 @boonebgorges
8 years ago

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

In 32383:

Improve performance of loop detection in _get_term_children().

Using an array keyed by term_id allows us to use isset() rather than the
slower in_array(). In addition, it lets us avoid the use of wp_list_pluck()
on large arrays, and helps us to avoid arrays that are unnecessarily large due
to duplicate entries.

Fixes #32144 for 4.2 branch.

Note: See TracTickets for help on using tickets.