Problem/Motivation
This function's doxygen claims that its results are statically cached but thats untrue because we later use $conditions to load the vocab. Such loads are prohibited from using entity cache.
Proposed resolution
Here we add a static cache in order to efficiently convert from machine name to vid.
Remaining tasks
re-roll and review patch
User interface changes
none
API changes
implement static cache for taxonomy_vocabulary_machine_name_load() that the documentation has always claimed exists.
Original report by @moshe weitzman
This function's doxygen claims that its results are statically cached but thats untrue because we later use $conditions to load the vocab. Such loads are prohibited from using entity cache.
Here we add a static cache in order to efficiently convert from machine name to vid.
Comment | File | Size | Author |
---|---|---|---|
#14 | fix_static_cache_in-965652-14.patch | 712 bytes | joelpittet |
#14 | interdiff.txt | 396 bytes | joelpittet |
#7 | vocab_static-965652-7.patch | 919 bytes | mrjmd |
#4 | vocab_static.patch | 1.54 KB | moshe weitzman |
#2 | vocab_static.patch | 1.67 KB | moshe weitzman |
Comments
Comment #1
catchsubscribing.
Comment #2
moshe weitzman CreditAttribution: moshe weitzman commentedSimplified a bit per catch feedback.
Comment #3
catchSo this looks good but it introduces an inconsistency between vocabularies and terms, since we have http://api.drupal.org/api/drupal/modules--taxonomy--taxonomy.module/func... which resets drupal_static() then the controller static there, while in this patch the controller resets drupal_static().
This as much points to the complete lack of standards we have for resetting static caches in general, which is #581626: Use a consistent/clean pattern for using $reset or drupal_static(). In general I think we should avoid introducing the inconsistency, but I'm not sure which of the controller vs. a helper doing this I prefer.
Comment #4
moshe weitzman CreditAttribution: moshe weitzman commentedNow without inconsistency
Comment #5
thedavidmeister CreditAttribution: thedavidmeister commentedThis should be answered by #1577902: [META] Remove all usages of drupal_static() & drupal_static_reset() in D8 or 9, so don't feel too bad about whatever happens here ;)
For D7 though, unfortunately:
Comment #6
thedavidmeister CreditAttribution: thedavidmeister commentedComment #7
mrjmd CreditAttribution: mrjmd commentedHere's a reroll. Adding the static caching to taxonomy_vocabulary_get_names() has already been committed, this just resets it during save and calls it from machine name load now.
Comment #8
joelpittetThis looks great, nice work:)
Comment #9
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedIt looks to me like taxonomy_vocabulary_save() already does this, no? Since it calls taxonomy_vocabulary_static_reset() whenever a vocabulary is saved, and that function clears this static cache.
I'm not sure why this is an improvement - doesn't the call to taxonomy_vocabulary_get_names() just mean an extra database query on some pages that don't require it?
I also don't understand what "Convert $name to $vid in order to avoid automatic cache miss" means exactly? (perhaps it is part of the answer to the above...)
Comment #10
joelpittet@David_Rothstein re #9 you are right in most cases it does through
taxonomy_vocabulary_static_reset()
Hopefully$vocabulary->vid
would exist and there is not an empty$vocabulary->name
or it would fail to flush the static names cache.I've been using this, I think I had a cache miss that this fixed but since I forgot to mention that in my RTBC, I forget the details. My horrible commit message 'taxonomy cache miss fix patch for core', I'll recind my RTBC and let someone else explain.
My only guess is that when you use the
$conditions
without$vids
theentityCache
is totally ignored.Comment #11
joelpittetActually I think that is is exactly what it was, without the
$vids
theentityCache
is ignored, so we do that small SQL query that is likely static cached and skipped in hopes to avoid always missing entityCache in the controller.Comment #12
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedBut the only case where it doesn't call taxonomy_vocabulary_static_reset() is when it doesn't actually save anything. So in that case there's no need to clear the static cache, right?
Ah, I see - it's because https://api.drupal.org/api/drupal/includes!entity.inc/function/DrupalDef... does:
(and in this case, $conditions is passed without $ids)
It seems to me like there may be room for the entity API to improve performance here (or at least make it easier for individual entities to improve performance in cases, like this one, where they know the $conditions are just as unique as the IDs are). Might be a better approach to consider. Because with the patch in this issue, it only really helps you if you're loading the same vocabulary more than once per request... if you're not then it's still just doing an extra database query via taxonomy_vocabulary_get_names() for no benefit.
Comment #13
joelpittet@David_Rothstein I think you are right on the first point.
Though unlikely there is a code path that has some undefined $status variable and skips it like I was mentioning.
Though that's unrelated and I think you're right, it's not needed.
It would be nice to know if the conditions are unique and magically get a cache hit, as long as single condition is a unique field or a pair together is a unique pair, etc. I don't think I could solve that in a generic enough way though...
I am loading the same vocabulary on through menu items, this issue may help #1978176: Build menu_tree without loading so many objects. It's easy to see when rebuilding the menu()/clear cache.
Comment #14
joelpittetI'm not convinced we can get a generic version on the entity controller for this.
Here's the patch without the redundant extra static clear.
Comment #15
JGReidy CreditAttribution: JGReidy as a volunteer commentedPatch #14 helped me get rid of warnings like this:
generated by this patch to entity:
array_flip_warning_in-2487462-21.patch
The problem was that taxonomy_vocabulary_machine_name_load calls taxonomy_vocabulary_load_multiple with a NULL instead of FALSE for the first argument (Drupal 7.54).
This patch #14 replaces that call to taxonomy_vocabulary_load_multiple.