CRM-20621 : manage tags: the tag usage count is not accurate #10441
Conversation
monishdeb
commented
May 30, 2017
•
monishdeb
commented
- CRM-20621: manage tags: the tag usage count is not accurate
|
looks good. count is accurate and query is compatible with mysql 5.7 |
|
@monishdeb can you please fix the failing test? |
|
@civicrm-builder retest this please |
|
@monishdeb and @lcdservices I know it's been a year since this was merged but I'm hitting a major performance issue with it on my sandbox. Clicking on a tagset with a few dozen tags in it takes 8 seconds to load, I can imagine it would take significantly longer on a live site with a few hundred tags in a set. The major bottleneck appears to be the But reading the code, it's also now calling up |
| 'usages' => civicrm_api3('EntityTag', 'getcount', array( | ||
| 'entity_table' => array('IN' => $usedFor), | ||
| 'tag_id' => $dao->id, | ||
| )), |
There was a problem hiding this comment.
I'm thinking we should get rid of this and not send usage data with the main payload. We can switch to lazy-loading it when clicking on an individual tag in the UI.
There was a problem hiding this comment.
On second thought, lazy loading is complicated because more than one tag can be selected at once. Is it really impossible to get this data from a join in the query?
| ORDER BY name"; | ||
|
|
||
| // fetch all child tags in Array('parent_tag' => array('child_tag_1', 'child_tag_2', ...)) format | ||
| $childTagIDs = CRM_Core_BAO_Tag::getChildTags(); |
There was a problem hiding this comment.
Is this absolutely necessary? If we don't need the usage data can we re-add the LEFT JOIN civicrm_tag child ON child.parent_id = tag.id back to the query? That seems to be the only purpose of loading this massive array into memory.
There was a problem hiding this comment.
Problem with the left join is that it doesn't look deep down the hierarchy to retrieve level-N child tags, reason why I need to introduce this new fn to fetch the tag tree via recursive search. However, we can avoid this lookup on each call by storing the $childTagIDs array in a Civi::static variable? And need to ensure that whenever a tag got deleted/moved in the tag tree this static variable got repopulated.
There was a problem hiding this comment.
This function, by design, does not need to look deep within the hierarchy. It only needs to know whether the tag has children or not. 1 level deep is fine.
| if (!empty($_REQUEST['is_unit_test'])) { | ||
| return $result; | ||
| } | ||
|
|
There was a problem hiding this comment.
We could remove this hack by making the CRM_Admin_Page_AJAX::getTagTree() function a one-line wrapper and move the logic to a BAO function that would do the work and could be more elegantly unit-tested.
|
@colemanw FWIW we're using this on NYSS where some sites have ~2000 tags, and we have not noticed a performance hit on that page. I'll do some more testing to confirm, but it's been in production with this change for quite some time without complaint. |
