Hello,

When calling taxonomy_get_term_by_name(), the resulting term will be cached again and again on database, even though it is already in the cache table.

The broader issue is that entityCacheLoad(), when called with $conditions instead of $ids (e.g. in this case, $conditions = array(vid=xx and name=xx)), will not find the cached entry (since indexed via the indexKey), and will cache the results every time.

So each time an entity is loaded via $conditions, a cache_set will be done. Not helping performance, especially if your cache tables are still in database.

I know $conditions are apparently deprecated, but some core functions still use that, e.g. taxonomy_get_term_by_name.

A patch is incoming.

CommentFileSizeAuthor
#8 cache_entities_by_id-1670848.8.patch969 bytestorgosPizza
#4 only_cache_entities_loaded_by_id-1670848-4.patch1.31 KBcolan
#1 entitycache-remove-unnecessary-setcaches-1670848.patch1.32 KBndeschildre
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ndeschildre’s picture

ndeschildre CreditAttribution: ndeschildre commented
Status: Active » Needs review
FileSize
entitycache-remove-unnecessary-setcaches-1670848.patch1.32 KB

The attached patch will not cache entities loaded in entityCacheLoad with $conditions, since the cache entries cannot be used by calls of entityCacheLoad with $conditions.

johnv’s picture

johnv
Primary language Dutch
CreditAttribution: johnv commented
Status: Needs review » Reviewed & tested by the community

This patch reduces the amount of Queries on my pages a lot.
Tested it by :
- show page: show number of queries with devel_querylog
- enable patch
- refresh page: show number of queries with devel_querylog
- rename a term
- refresh page: change of term is reflected.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Reviewed & tested by the community » Needs review

The patch in #1 needs to be adjusted for the coding standards. Comments need a space between the // and the comment text, and if statements need a space in-between the if and the next parenthesis.

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
CreditAttribution: colan commented
FileSize
only_cache_entities_loaded_by_id-1670848-4.patch1.31 KB

Cleaned it as per the coding standards.

johnv’s picture

johnv
Primary language Dutch
CreditAttribution: johnv commented
Status: Needs review » Reviewed & tested by the community

The new patch applies cleanly and works as expected.

marcelovani’s picture

marcelovani
Primary language Portuguese, Brazil
Location London
CreditAttribution: marcelovani commented
Issue summary: View changes

Patch #4 works well. Thanks.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

This generally makes sense.

+++ b/entitycache.module
@@ -112,7 +112,15 @@ class EntityCacheControllerHelper extends DrupalDefaultEntityController {
+          if (empty($queried_entities_by_id) == FALSE) {

I think it would be more readable to just do !empty() here.

torgosPizza’s picture

torgosPizza
he/him
Primary language English
Location Portland, OR
CreditAttribution: torgosPizza commented
FileSize
cache_entities_by_id-1670848.8.patch969 bytes

Here is a re-roll of the patch from #4 with the change suggested in #7.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented

We just had another project that was incorrectly running cache_set() because of this problem, and the patch worked great.

skwashd’s picture

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

Thanks for the patch. I have committed #8 and it will be included in the next release.

skwashd’s picture

skwashd CreditAttribution: skwashd commented

  • skwashd committed dbf1820 on 7.x-1.x authored by ndeschildre 
    Issue #1670848 by torgosPizza, colan, ndeschildre: Fixed Unnecessary...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.