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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

subscribing.

moshe weitzman’s picture

FileSize
1.67 KB

Simplified a bit per catch feedback.

catch’s picture

So 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.

moshe weitzman’s picture

FileSize
1.54 KB

Now without inconsistency

thedavidmeister’s picture

Assigned: moshe weitzman » Unassigned
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll
Related issues: +#1577902: [META] Remove all usages of drupal_static() & drupal_static_reset()

This as much points to the complete lack of standards we have for resetting static caches in general,

This 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:

tdm:d7 thedavidmeister$ git apply vocab_static_1.patch 
vocab_static_1.patch:22: trailing whitespace.
  
error: taxonomy/taxonomy.module: No such file or directory
thedavidmeister’s picture

Issue summary: View changes
mrjmd’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
919 bytes

Here'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.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Performance

This looks great, nice work:)

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review
@@ -452,6 +452,7 @@ function taxonomy_vocabulary_save($vocabulary) {
 
   unset($vocabulary->original);
   cache_clear_all();
+  drupal_static_reset('taxonomy_vocabulary_get_names');

It 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.

 function taxonomy_vocabulary_machine_name_load($name) {
-  $vocabularies = taxonomy_vocabulary_load_multiple(NULL, array('machine_name' => $name));
-  return reset($vocabularies);
+  $names = taxonomy_vocabulary_get_names();
+  if (isset($names[$name])) {
+    // Convert $name to $vid in order to avoid automatic cache miss.
+    return taxonomy_vocabulary_load($names[$name]->vid);
+  }

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...)

joelpittet’s picture

@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 the entityCache is totally ignored.

joelpittet’s picture

Actually I think that is is exactly what it was, without the $vids the entityCache 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.

David_Rothstein’s picture

@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.

But 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?

Actually I think that is is exactly what it was, without the $vids the entityCache 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.

Ah, I see - it's because https://api.drupal.org/api/drupal/includes!entity.inc/function/DrupalDef... does:

  // Load any remaining entities from the database. This is the case if $ids
  // is set to FALSE (so we load all entities), if there are any ids left to
  // load, if loading a revision, or if $conditions was passed without $ids.
  if ($ids === FALSE || $ids || $revision_id || ($conditions && !$passed_ids)) {
    // Build the query.
    $query = $this->buildQuery($ids, $conditions, $revision_id);
    $queried_entities = $query
    ->execute()
      ->fetchAllAssoc($this->idKey);
  }

(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.

joelpittet’s picture

@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.

$vocab = new stdClass;
$vocab->vid = 1;
$vocab->name = '0';
$vocab->machine_name = 'empty';
taxonomy_vocabulary_save($vocab);

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.

joelpittet’s picture

I'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.

JGReidy’s picture

Patch #14 helped me get rid of warnings like this:

Debug: The second argument passed to entity_load() must be FALSE or an array. in entity_load() (line 8028 of /code/includes/common.inc).

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.