Return early in CacheTagsChecksumTrait::isValid() (and possibly elsewhere) if there are no tags to check

Hi!
I hope is fine, i have added the snippet of @wim Leers in https://www.drupal.org/project/drupal/issues/3421881#comment-15456992 📌 Track cache tag queries separately in performance tests Active in here :)

The code looks fine (😇), but the comment doesn't quite make sense to me: the comment is more complicated than the logic here 😄 It's much simpler: if there's no cache tags, then there is no cache tag to invalidate, hence it's always valid.

I think the change here is good, but also just realised we could do the same early return in the decorator in 📌 Track cache tag queries separately in performance tests Active too.

Agreed on the comment, it is always the hardest part :D :D , but i liked your last sentence so i have used it ;)

@cat, you mean in core/modules/system/tests/modules/performance_test/src/Cache/CacheTagsChecksumDecorator.php::isValid(), correct?

if we do as well there the

    if (empty($tags)) {
      return TRUE;
    }

should we also do a $this->logCacheTagOperation([], 0, 0, CacheTagOperation::isValid); (or smth like that before the return? Or there is no need to log the operation?

Thinking about it - we should call the child class so it's a proper decorator, but skip logging because we know there's nothing to log. Probably needs a separate comment. The empty cache tag operations create a lot of noise in grafana tempo, not having them there will make it easier to find things we need to change.

📌 Track cache tag queries separately in performance tests Active just landed which means we can do that change here now, it'll mean also adjusting the assertions we just added.

Went ahead and implemented #9. I also noticed that we were still sending some cache tag queries to grafana, so moved that logic to a helper so it's the same in both places we check.

Just a nit — this looks great!

Also, am I missing something, or is this actually saving DB queries?! Nonsensical queries like

SELECT [tag], [invalidations] FROM {cachetags} WHERE [tag] IN ()'

🙈

It doesn't save any real database queries, we already check that there is something new to query in CacheTagChecksumTrait::calculateChecksum and similar places, so it's mostly removing noise when profiling (either traces in grafana tempo or less function calls to look at in xhprof and similar too) and a small micro-optimization.

I noticed we're only changing isValid(), what about the other two methods we're tracking in our perfrmance tests?

Re #11, not really:

    $query_tags = array_diff($tags, array_keys($this->tagCache));
    if ($query_tags) {
      $tag_invalidations = $this->getTagInvalidationCounts($query_tags);
      $this->tagCache += $tag_invalidations;
      // Fill static cache with empty objects for tags not found in the storage.
      $this->tagCache += array_fill_keys(array_diff($query_tags, array_keys($tag_invalidations)), 0);
    }

If $tags is empty, then so will $query_tags so we weren't running a query for that. It was showing up in OpenTelemtry, though, because we were tracking all calls in the decorator. This patch fixes that so you'll see a significantly reduced amount of them being tracked.

Committed and pushed 02c07301fc to 11.x and 86594c44d6 to 10.3.x. Thanks!

Sooooo... a follow-up for getCurrentChecksum and invalidateTags then? :D

More specifically, I was thinking about:

• Only the top of CacheTagsChecksumDecorator::invalidateTags() because CacheTagsChecksumTrait already short-circuits if the list of tags is empty.
• The top of CacheTagsChecksumDecorator::getCurrentChecksum()
• The top of CacheTagsChecksumTrait::getCurrentChecksum() because it runs a bunch of code that will end up doing nothing if the list of tags is empty

Good point, not sure if those are also polluting the grafana traces but even if they're not it makes some things easier to follow, opened 📌 Return early from more cache tag operations Active .

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