taxonomy_get_tree() memory issues

Here are a few suggested solutions. Hope this help.

• Provides an autocomplete form field instead of a drop down if a taxonomy is bigger than a predefined number of terms. Similar to http://drupal.org/project/batax
• On content type page or taxonomy page. Offer 2 options: 1-Use drop down. 2-Use autocomplete.
• Collapse the taxonomy fieldset by default. Then load it when extended by user. Using AJAX/AHAH.
• Loading a drop down menu populated with taxonomy title but not all terms. Then when user select a taxonomy title load only the term associated with the taxonomy. Using AJAX/AHAH. Similar to http://drupal.org/project/taxonomy_widget or http://drupal.org/project/hierarchical_select

My vote goes for the first solution. Because it seems it would answer all type of user needs. The drop down menu would be the default value. But user could set it to change if a taxonomy is bigger than a predefined number of terms. Plus user could set a number of terms to trigger an autocomplete.

This can still get in as a usability/performance improvement in Drupal 7.

I'd say that we should fold into core the http://drupal.org/project/batax module

This issue exists since like ever.

I like the idea to fold the batax module into core.

Subscribing!

Crashing people's sites just because they happen to have a decent amount of content is a bug, and a critical one. If we were loading the entire contents of the node table every time you go to search/node, admin/content, node/n/edit and taxonomy/term/n/edit this wouldn't have been downgraded. But I won't play status pong.

Whats happenning is the same as what happens in 6. I disagree with the criteria that "if it is in 6, it is not critical". Bugs should be evaluated as if they are new. catch is saying that if we had this same bug for nodes, it would be critical. taxonomy is a similar fundamental system. Looks at the paths that catch posted. Those are fundamental paths that won't even load if you have a lot of terms.

Anyway, the point is not to argue about status. I propose that the best fix is to move code from http://drupal.org/project/batax into core. We'd just be adding a new field api widget. Please comment on the feasibility of committing such a patch. If it is plausible (i know there are no guarantees), we'll prepare a patch.

subscribe

Also opened #698918: Reduce memory usage from taxonomy_get_tree() when parent is specified when I remembered that even if you specify $parent in taxonomy_get_tree() it still loads the entire vocabulary into memory. I'm not sure if that issue is 100% viable yet, but if it is then that'd mean we don't have to remove the last four words of this issue title ;)

IIUC, this is about building a too-large form element in content/admin and search/node/.

If so this is no longer an issue due to #693362: taxonomy_form_all() is dangerous removing the filter-by-term form elements.

Marking as duplicate.

We still call this for the term parents when editing nodes, and in taxonomy_field_allowed_values(). Retitling for the remaining issues.

Forcing the widget sounds good.

Threshold, I think this should be a variable (although not necessarily an exposed one). We could have a taxonomy_autocomplete_threshold and a taxonomy_vocabulary_term_counts variable -the latter updated on taxonomy_term_insert. When the latter crosses the formerf or any particular vocabulary, update instances?

I like the idea of showing the ancestry. Another option if that's hard, would be the [tid: 123] approach used by node and user reference.

Yeah 150-200 seems like a sensible threshold to me too.

Here we go!

H,,. I wonder if we should check that count during cron instead of adding a query on every save. That sort of query is painful when creating terms en masse like a data migration. At least that auto-check for vocab size should be able to be disabled.

@Moshe: this is a no return theshold- it's checked until the threshold is crossed, then never checked again. So for migrations it would only affect the first 200 terms inserted into a vocabulary - once those are in, the threshold is passed, and it shouldn't be checked again.

For migrations, it'd be enough to set $conf['taxonomy_autocomplete_threshold'] = 1; - then you'd get only one COUNT query per vocabulary. Although that's a bit obscure, I'm not sure we need any other special handling here. Would prefer we didn't do it on cron, since then you're counting for a 100 term vocabulary that never changes in perpetuity.
However, variable_set('taxonomy_vocabulary_term_counts', $counts); - this variable should be renamed to taxonomy_vocabulary_threshold_exceeded or something - it's not easy to tell what's going on there.

@bangpound - what's left to do here otherwise?

@catch. thanks. my concerns are gone. i'd be happy to see this move ahead.

@all: If you want to test patches. Using Drupal 7 alpha 3 or more recent the steps to 'assigning vocabularies to content types' has changed.

Before you can use a new vocabulary to classify your content, a new Taxonomy term field must be added to a content type on its manage fields page. When adding a taxonomy field, you choose a widget to use to enter the taxonomy information on the content editing page: a select list, checkboxes, radio buttons, or an auto-complete field (to build a free-tagging vocabulary). After choosing the field type and widget, on the subsequent field settings page you can choose the desired vocabulary, whether one or multiple terms can be chosen from the vocabulary, and other settings. The same vocabulary can be added to multiple content types, by using the "Add existing field" section on the manage fields page.

Source: Drupal 7 online help button. #overlay=admin/help/taxonomy

I'm currently testing patch #28. Will post my result here.

The following is a note to myself for future patch testing.

New steps to create large vocabulary. Then attach it to a content type.

Create new vocabularies with 'Devel Generate taxonomy'.

Go to admin/structure/types

Click on 'manage fields' link beside 'Basic page'.

Fill the 'Add new field' row.

For 'Field type' select 'Term reference'.

For 'Widget' select 'Select list'.

Click on save button. Find attached screenshot to clarify.

On the next screen FIELD SETTING. Select the Vocabulary 'Tags'. Note that the current 'Devel Generate taxonomy (devel-7.x-1.0-beta1.tar)' module as a glitch. All new taxonomy generated with 'Devel Generate taxonomy' are added to the 'Tags' vocabulary. I have open another issue about that at #750842: 'Devel Generate taxonomy' does not generate expected number of terms

On the next screen leave all to default. Click on SAVE SETTINGS button.

On the next screen click on SAVE CONTENT TYPE button.

Go to /user/1#overlay=node/add/page

Follow instructions on screen to create a new page.

Onopoc: The patch is not finished yet. I don't think there's anything to profile yet, and I will likely not do anything for taxonomy term reference fields. I'm targeting taxonomy term parents in the term edit form. Otherwise, Drupal needs to urge the user to use autocomplete widgets in taxonomy term reference fields when the total number of allowed values exceeds the threshold.

Subscribe, similar approach recommended for #763380: Do not use taxonomy_get_tree() in taxonomy_overview_terms()

"Critical: When a bug renders a module, a core system, or a popular function unusable. ... to be fixed immediately because the software is not usable at all."

This isn't critical. The software works in a majority of cases.

Not to dismiss it, this issue should be fixed.

We have major priority now.

I was working on improving the performance of the taxonomy_get_tree function, which is used in many places (e.g. for loading the options of a select list or for the term overview page). This patch does not affect any form elements, which try to render too many terms at a time, so not definitely sure if it's the right place to post the patch.

As I pointed out in my previous comment, the recursion and the array_merge calls take up a long time. Thus I replaced the whole recursion with iterations, which really speeds up the complete function. Performance improvements can be easily seen with bigger taxonomies and multi-parents. On the downside, the code gets a bit more difficult to read.

I tried to use my taxonomy with D7 and was facing heavy performance problems (on a server with an Intel Core i7 920 and 12 GB RAM). The vocabulary has around 14.000 terms and some multi-parents.

First, the term overview crashes -> fatal error: memory size exhausted: this is caused by taxonomy_get_tree and the huge table.
Second, the term edit form takes around 12 seconds just for loading the form, same problem here: taxonomy_get_tree
In general, every time taxonomy_get_tree gets called, I have to wait for a long time. Since taxonomy_get_tree fetches full term objects, the function is much slower than in D6 (full entity loading has been added in #870528: taxonomy_get_parents(), taxonomy_get_children(), and taxonomy_get_tree() do not return a full term objects). When switching back to the old behaviour of taxonomy_get_tree, without the entity loading, the execution time goes back to 1 second (still bad).

marking this issue as critical, because the performance is much worse than in D6 and the problem will affect other sites as well.

overview of the execution time of taxonomy_get_tree:
with entity loading: 12sec
without entity loading: 1,1 sec
without entity loading and without recursions (patch comment #47): 0,45sec (additionally removing the cloning of terms reduces the PHP memory peak from 160mb to 62mb)

not sure what the best solution is. having incomplete objects is not really a good idea, but on the other side we need a fast function for building the forms and overviews.

further, multi parents are currently broken, see #899258: Multi parents broken

>demoting from critical because there is nothing new here, this always was so.
I'm not happy about another critical, but I disagree. As mh86 noted above, the same taxonomy was working fine in previous drupal versions - so that's a regression :(

A that bad performing taxonomy_get_tree() seriously breaks dealing with any term, as the function is used during term editing + overview. Improving it to only entity-load the terms really needed would be a first start. However I doubt that will suffice as its often called for all vocabs too.

Apart from that, the taxonomy_get_tree() does not really return a tree. Moreover it looks like it is basically used for generating a flat list of the tree for overviews / selects. So a separate function optimized for generating that list (= without full entity loading) would make sense to me - maybe it should even return arrays instead of objects to make a clear distinction to full entity objects. taxonomy_get_tree() could then be fixed up in core to really return a tree, however I think there is no use-case for that in core.

trailing white spaces

trailing white spaces

Otherwise I think this is a good idea. Changing the comments on taxonomy_get_tree() would be good, to indicate that people should not use it anymore for the hierarchy listing use case. And a hint like @see taxonomy_get_entries().

And we should deal with the threshold for the term overview page in a separate issue.

Powered by Dreditor.

Changing the comments on taxonomy_get_tree() would be good, to indicate that people should not use it anymore for the hierarchy listing use case. And a hint like @see taxonomy_get_entries().

As stated in #49,

Since taxonomy_get_tree fetches full term objects, the function is much slower than in D6...

...it is therefore a new bug, a regression as it now takes longer than D6. The results are different, and worse.

In agreement with chx this is not critical vocabularies have always behaved in this way.

Major does not mean something won't be fixed. This bug does anot render the system unusable, however it does have significant repercussions therefore according to this http://drupal.org/node/45111 the issue is major not critical.

Please don't get anxious about the priority. This particular issue is a hard one to classify. It never stopped Drupal 5 or Drupal 6 from being released, so why stop Drupal 7? Meanwhile, it's clear that Drupal 7 fanciness exacerbates the bad situation.

As of April in San Francisco, Webchick and I (and others) agreed that this issue is major and not critical. However, real world scenarios with the upgrade path could obligate us to escalate the priority of the issue.

Will this be a critical issue with no functioning patch? Or a major issue with a golden patch?

According to the evidence presented, mh86's function taxonomy_get_entries() is much faster. He says it's harder to understand, but taxonomy_get_tree() is ALREADY opaque to me and taxonomy_get_entries() is a bit clearer to understand. So this is a good accomplishment!

Drupal core does not need debris and legacy functions. taxonomy_get_entries() is not going to get a pass while taxonomy_get_tree() also gets to stick around. Ultimately, the RTBC solution is one that FIXES or REPLACES taxonomy_get_tree().

The array returned by taxonomy_get_tree() is a real trick to use. It's not a flat array. Is it exceptional enough that we don't need to think about entities here? Parent term is not a field,
so the way hierarchy is implemented in Drupal 7 is already a bit of a relic.

Do all our tests pass if the body of mh86's taxonomy_get_entries() is pasted over the body of taxonomy_get_tree()? This is where I'd like to start.

I'll come back to the issue in about a day. I need to do more research about how taxonomy_get_tree() is used in core. I don't oppose having it return a lightweight representation of term names, IDs and hierarchy only. But I also don't know if that's possible. More tomorrow.

The patch from #60 replaces all occurrences of taxonomy_get_tree with the new taxonomy_get_entries function and all tests pass. Full entity loading was added after alpha6 (see #870528: taxonomy_get_parents(), taxonomy_get_children(), and taxonomy_get_tree() do not return a full term objects).

I'm still not sure, how the best solution should look like. Dealing with incomplete terms is not a good idea. On the other hand, the way core uses it for building forms, full terms mean a lot of overhead. This was the reason, why I added a separate function for this use case, but still with the tradeoff of no possibility to change terms on hook load, as chx mentioned.

Furthermore, if we return a lightweight representation of terms, we should think about returning term arrays (like $term['tid']...) instead of incomplete term objects. This would make it clearer to developers, that they are not dealing with real terms. On the other hand this would break much code in core and contrib.

Designing it this way sounds reasonable to me. The code stays simple and it is close to the previous implementation (except the replacement of the recursion). Combining List+Load in one function just required a few additional lines of code and both profit from the iteration implementation.

To give some performance numbers, tested the function on my local machine with 5.000 terms, simple hierarchy:

• original taxonomy_get_tree, with full entities: 2,7sec
• updated taxonomy_get_tree, with full entities: 2,3sec
• updated taxonomy_get_tree, just list: 0,2sec

At the moment core never requires full entities, therefore this change really improves performance.
I'm still thinking of the variable naming $list_only, which wasn't clear for me at the first glance. Maybe we should use something like $fetch_full_term_entities or $fetch_structure_only, what would you say?

You are right, there is no need for another static variable. I can update the patch tomorrow.

here the updated patch:

• removes static var for $term_entities and reuses the cache from the entity controller
• renamed $list_only in $load_entities (seems more intuitive to me) and made it default. Still I hope the list mode is sufficient for most places in core, but we'll see.

In my previous posts I forgot to mention, that this patch also fixes multi parents, which are currently broken in taxonomy_get_tree (term with multi parents only added once to $tree array).

Looks great to me as well.

This issue was the first mention of #870528: taxonomy_get_parents(), taxonomy_get_children(), and taxonomy_get_tree() do not return a full term objects I've seen, I would've argued strongly against that going in given the existence of this issue, but at least this alleviates the worst of it.

I ran into this as well. It's worth noting that this is not merely an edge case caused by very large vocabularies. If you attach fields to your taxonomy_term entities you can quickly reach 20, 30+ second node edit form page loads with vocabularies containing relatively few terms (e.g. 100 or 200 terms). The more fields, the slower the loads.

The patch in #73 looks good but it seems like a one-off for taxonomy_get_tree() when to me this seems like a problem for any large entity_load() operation that doesn't require full loading of fields. Is the recommendation here to not use the entity API when you know you don't need fields and will incur a serious performance hit for calling it? (Start looking around core and you will see many instances of taxonomy_term_load_multiple() and user_load_multiple() that will never need information beyond data from the entity base table...)

My first inclination was to add a new function entity_list() (and derivative wrapper functions) and corresponding flag to the entity controllers ->load() method that avoids calling ->attachLoad(). If this sounds reasonable I have the patch on hand.

@moshe
I'm in favour making the list mode as default option. I was reviewing the taxonomy_get_tree function calls and I don't see any place where it's is necessary to have full entities, but waiting for bangpounds opinion.

If the plan is to translate term names during term loading, it sounds reasonable to me to use full entities. In this case, we are not going to consider performance problems on the node edit form, as yhahn reported in #77. Nevertheless, a simple contrib module could override the allowed_values with a more efficient implementation.
If we just use the list mode in allowed values, translating should still be possible with some form altering (as it was done previous to D7). Compared to full entities, this is more efficient, but maybe requires more code?
So, both approaches have pros and cons, most important we get something like the efficient list mode for taxonomy_get_tree in, so that contrib modules can take advantage.
Concerning the expectations of the results returned by taxonomy_get_tree, I would rather assume, developers don't expect full entities, because this is completely new since alpha6.

I just discussed this with mh86 and wondered whether it is an option to disable attaching fields to the term object by default?

I guess attaching fields is that what makes entity load that more expensive than a simple load operation. But I think they are usually not needed. So we could go and not attach them by default, but provide an API function to do so on demand.

I took a closer look at the entity loading and I found two main issues, which make loading of full terms that slow:

1. In the current implementation and in the patch we call:
$term_entities = taxonomy_term_load_multiple(array_keys($terms[$vid]));
This ends up in a query with "... WHERE tid IN ( all term ids)", where retrieving the result set was extremely slow (even though the query execution time isn't that bad). Changing this line to:
$term_entities = taxonomy_term_load_multiple(array(), array('vid' => $vid));
helps a lot.

2. indeed, attaching fields to a term takes a lot of time, even if the vocabulary has no fields associated. Attaching them on demand, as fago suggested, would make sense.

With fixing these issues we can get taxonomy_get_tree working in a reasonable amount of time, even with entities. I'm going to update the patch as soon as I find time.

Patch no longer applied, re-rolled with the doxygen changes Moshe asked for, and defaulting the parameter to FALSE. Otherwise it's unchaged apart from updating to HEAD.

Part one of #85 is worth looking at, but I'm pretty happy with this overall as is.

Subscribe

Reviewed #87 - this fixes the performance problems I experienced on node edit pages that use vocabularies with many terms with fields.

FWIW, I agree w/ catch on #86/2 - local optimization seems to be preferable to introducing the concept of fieldless entities. #85/1 is an interesting point, but really material for a separate issue.

RTBC, here too.

Pre-emptive postgres un-breakage via webchick in irc.

Updated the patch from #93 with more comments on the loop.

Concerning #85/1: loading full entities from a voc with 2500 terms with current code took 1,2sec, with the $vid condition instead 0,85sec. But this can be addressed in a separate issue.

Needs a re-roll with git --no-prefix.

Reviewing the patch:
This is my fault, but taxonomy_term_data should be {taxonomy_term_data} IMO, makes it obvious it's a db table. Not a blocker, may be making up that code style too...

Comment additions look good.

Looks great.

Thanks all for your contributions. This is awesome :)

Yeah, actually, the change as-is might be back-portable since it didn't end up affecting the UI. It does incur an API change, but to a property that contrib authors shouldn't be using. Up to Gábor, of course.

subscribing.

#98: taxonomy_get_tree_556842_1.patch queued for re-testing.

For those who are trying to solve the slow down problem of hierarchical select, take a look at this link:

http://drupal.org/node/1059676#comment-4095278

Hope this helps.

Straight backport to D6, just stripped the entities stuff.

#286103: WSOD (blank page) on admin/content/node with freetagging taxonomy marked as dupe.

@phantomvish if it is working for you please, RTBC this issue

Here's an update that fixes some small issues with the phpdoc comment.

Testing this looks to be about 20% faster than the current taxonomy_get_tree() function in D6. However, I'm not seeing any difference in the memory profile with this patch. Reading the previous issues, it seems as if the expected improvement is only in CPU time, and not memory use. If that's the case, this is fine at RTBC, otherwise I think this needs work.

Committed, pushed, thanks.