Improve performance of event cache by moving to a whitelist

I was about to file a bug of my own on this subject. It does not seem right, that Rules-module uses the variable-table as its own cache...

We replicate the master-database to (a lot of) slaves using local memcache on each slave for their own private caching — to avoid database traffic. The incessant insertions/deletes from the master database cause us grief. If the rules_empty_sets is, indeed, a variable, then it should not be deleted each time a cache-clear is performed. And if it is a cache-item, then the value needs to live where the rest of the cache does (either in the default bin or in a bin of its own).

We'll try to use the proposed patch (for D7)... Thanks!

In reply to #2

Thank you very much for offering the patches. I have a question, however, about it. Instead of storing the value of rules_empty_sets in the variable-table, you place it in the cache (in the new cache_rules bin). But don't you need to explicitly clear the bin, when a cache-flush is requested?

And if it is not necessary, perhaps the entire rules_flush_caches hook can be eliminated?

If this is, indeed, the only thing in the new cache_rules bin, maybe, using the default cache bin is better?

I only did a quick test of applying the patch, clearing the cache, and running a rule. There were no apparent side effects of applying this patch.

Here's a bit of a re-roll of this patch, addressing some additional areas where the variable was being used.

This patch should work better.

Here's an interdiff of the last two patches:

interdiff rules-convert_empty_sets_variable_to_cache-1555634-9.patch rules-convert_empty_sets_variable_to_cache-1555634-10.patch
diff -u b/rules.module b/rules.module

--- b/rules.module
+++ b/rules.module
@@ -865,7 +865,7 @@
 
   // For invoking the rules event we directly acccess the global $conf. This is
   // fast without having to introduce another static cache.
-  if (!defined('MAINTENANCE_MODE') && !isset($empty_sets['event_name']) && $event = rules_get_cache('event_' . $event_name)) {
+  if (!defined('MAINTENANCE_MODE') && !isset($empty_sets[$event_name]) && $event = rules_get_cache('event_' . $event_name)) {
     $event->executeByArgs($args);
   }
 }

A different example where rebuild is still triggered on every request (!) is when you render exportable/configuration entities.

The reason for this is that the default rules event controller is not added for this, but the view event is still called by EntityAPIController. Because that event does not exist, it is not in empty set, nor is there a cache entry for it. Which results in a rules event rebuild on every request where such an entity is viewed.

That is, in my case, every page ;)

I'm not sure why rules explicitly keeps track of empty sets, instead of simply tracking which sets actually have rules in them?

Because that event does not exist, it is not in empty set, nor is there a cache entry for it.

Yep, but the problem here is modules invoking events that do not exist I think. I don't think we should optimize for that, but rather fix that.

I'm not sure why rules explicitly keeps track of empty sets, instead of simply tracking which sets actually have rules in them?

I agree that would make most sense, not sure why we started it the other way round. I think the thought was to not duplicate existing cache information, but we are doing that anyway (regardless of inversion).

Also, keeping track of existing items would help with #1993932: Add support for event settings, where the negated approach cannot be used any more.

ok, attached patch moves to an whitelist and removes the variable_del() during cron run. I think it's unnecessary as it should be enough to update the variable when caches are already rebuilt anyway or a reaction rule got configured.

Patch attached.

#1993932: Add support for event settings got committed, what means we really need to do this now. Re-rolled patch -> let the bot run.

Committed.

This totally was the wrong fix.

The problem was using variable_get/_set in the first place not deleting it at cron or moving to a white list.

#2190775: Misuse of variables table with rules_event_whitelist is the follow-up. Please please please get this in.

Thanks!