This patch adds static caching to taxonomy_get_term. It is hard to find examples of where this makes a difference in core, but the contrib repository is full of examples of taxonomy_get_term being called directly (which is correct), and the case often arises where it is called more than one time with the same term. Rather than put the burden of caching or optimizing on the contrib module developers, I believe that the right place is in taxonomy_get_term.

CommentFileSizeAuthor
#17 taxonomy_get_term_cache_2.patch2.83 KBrobertDouglass
#6 taxonomy_get_term_cache_1.patch965 bytesrobertDouglass
#3 taxonomy_get_term_cache_0.patch964 bytesrobertDouglass
taxonomy_get_term_cache.patch966 bytesrobertDouglass
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

robertDouglass’s picture

robertDouglass CreditAttribution: robertDouglass commented

This patch can be used for 4.7 and 4.6 as well.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Needs work

is_null is slow, use isset instead.

robertDouglass’s picture

robertDouglass CreditAttribution: robertDouglass commented
FileSize
taxonomy_get_term_cache_0.patch964 bytes

chx hath spoken

robertDouglass’s picture

robertDouglass CreditAttribution: robertDouglass commented
Status: Needs work » Reviewed & tested by the community
m3avrck’s picture

m3avrck CreditAttribution: m3avrck commented
Status: Reviewed & tested by the community » Needs work

Shouldn't that be:

if (!isset()) {}

Missing the *!* part?

robertDouglass’s picture

robertDouglass CreditAttribution: robertDouglass commented
FileSize
taxonomy_get_term_cache_1.patch965 bytes

doh! Caught by the double negative. Thanks.

robertDouglass’s picture

robertDouglass CreditAttribution: robertDouglass commented
Status: Needs work » Reviewed & tested by the community
Dries’s picture

Dries CreditAttribution: Dries commented

It would be good to look at the other (get-) functions in taxonomy.module to investigate which do caching and which don't. There are quite a few of these and making their behavior inconsistent is not adviced.

robertDouglass’s picture

robertDouglass CreditAttribution: robertDouglass commented

Here's a summary of the taxonomy_get_* functions and their caching status:

With caching

  • taxonomy_get_tree
  • _taxonomy_term_children
  • taxonomy_get_vocabulary
  • taxonomy_term_count_nodes

Without caching

  • taxonomy_get_vocabularies
  • taxonomy_get_related
  • taxonomy_get_parents
  • taxonomy_get_parents_all
  • taxonomy_get_children
  • taxonomy_get_synonyms
  • taxonomy_get_synonym_root

What level of consistency are you looking for, Dries? How much static caching is too much? I think the case at hand (taxonomy_get_term) is a clear win.

m3avrck’s picture

m3avrck CreditAttribution: m3avrck commented

Hmmm, I think I agree with Dries, consistency is best.

While this patch addresses taxonomy_get_term(), I think that's an isolate case--I know we are using that function a ton, just because :-)

However, that is not to say another site might be using another function that currently isn't cached -- in that case another patch would come up later to add it in.

I don't see any reason not to use the same static !isset() for all them --- speeding them all up consistently is a clear, easy, and future proof win :-)

robertDouglass’s picture

robertDouglass CreditAttribution: robertDouglass commented

I'm not convinced that adding static caching to all of them is worth it. I have a suspicion that we'd be increasing the memory footprint by storing copies of variables that would otherwise be released. Like you said, taxonomy_get_term gets called a lot (and not just in this specific case), whereas some of the functions listed may not. I'd like some finer grained guidelines (or benchmarks) before adding static to every lookup function. And then, what about the rest of Drupal? There are lots of other cases where things get looked up from the database but not static cached. While I'm all for consistency, I'm also for incremental change =)

robertDouglass’s picture

robertDouglass CreditAttribution: robertDouglass commented

And just to be a devil's advocate: aren't all of these lookup functions operating on what is essentially the same data model? Couldn't they be sharing the same static cached data structure? If we cache taxonomy_get_tree, and taxonomy_get_term, aren't we caching the same information twice in a lot of cases?

If we want to be consistent about caching, I would advocate something more radical than just adding a static array to the beginning of all those functions listed. I'd advocate defining the taxonomy data structure (taxonomy_get_tree comes pretty close already) and centralizing the cache so that the structure gets built a maximum of one time per request. This would also make a *sweet* point of departure for things like memcache.

robertDouglass’s picture

robertDouglass CreditAttribution: robertDouglass commented

but the attached patch reduces queries now =)

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented

I read through the patch, and think this is okay.

Dries’s picture

Dries CreditAttribution: Dries commented

Patch looks OK but let's document it in the phpDoc/API documentation?

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Reviewed & tested by the community » Needs work

Agreed.

robertDouglass’s picture

robertDouglass CreditAttribution: robertDouglass commented
FileSize
taxonomy_get_term_cache_2.patch2.83 KB

PHP Docs updated for all functions that do static caching.

robertDouglass’s picture

robertDouglass CreditAttribution: robertDouglass commented
Status: Needs work » Reviewed & tested by the community
Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)