rebuilding the cache is slow due to integrityChecks

I experienced similar problem with 7.x-2.0-rc2. Looks like it has been fixed in 7.x-2.0.

I reconfirm that my issue is not related to this issue.

I've noticed a number of slow queries to the cache_rules table that use the "For Update" clause. For Update locks tables by making them temporarily read only, which could explain your slow writes. It may also slow down your selects, since one select for update will have lock contention with any current select for update and would need to wait until the first lock is released.

I'm experiencing massive slow down in the Rules Admin UI whenever I try and edit or save a rule. My suspicion is that its all the integrity checking thats slowing things down. I have quite a lot of rules configured and as I have been adding more rules, the slowdown has been getting worse and worse. If I look at show full processlist in Mysql when this slowdown is happening, it shows a very long running Sleep command (it stays there for upto 60 seconds). I had to increase my PHP max execution time to 2 minutes now to even allow edited rules to finish saving.

Created a separate ticket for my issues as I think its different from this one #1383522: Huge performance impact when using lots of 'entity has field' checks.

hm, the interaction with the cache table happens only via the abstraction of the caching system. Thus, I don't think there is anything special in rules. However, yes each time a rules is saved the rules cache gets cleared and when the cache is refreshed all rules are integrity-checked and put into the check.

The integrity check appears to be rather costly and could probably optimized. Note, that this is not critical as it won't run at rule-evaluation time, but still it sucks if the admin UI gets slow.

Just updated one of our existing sites to the latest rules version and this definitely isn't usable any more.
We've about 1500 rules, lots had entity has field, I've rewritten all of them to data is as suggested in #1383522: Huge performance impact when using lots of 'entity has field' checks.. But still, the automated generation of the necessary rules takes forever.
Before the update we were able to generate 1375 rules in under a minute, now it doesn't come to an end.
If've played around a bit:
I set $rule->is_rebuild on the generated rule and removed rules_clear_cache() in RulesPlugin::save() in favour of a final cache generation at the end of our script .
Now the 1375 rules get imported in 16.95 sec, the rebuild takes 884.05 sec.
If you imagine that the rules_clear_cache() in RulesPlugin::save() would be still in place, just the import of the last ~6 rules would take about 1 hour.
And every time our customer edits a rule he had to wait ~10 minutes?!
Currently I see two parts of a possible solution:

• rules_clear_cache() in RulesPlugin::save() has to be replaced by something to update/add the current saved rule in the cache.
• The integrity check shouldn't run that often - I'd suggest to run it in RulesPlugin::save() and by cron.
  But e.g. not for all rules in RulesPluginUI::buildContent() (which currently also clears the whole cache again if an element is dirty...)

I'll start creating a patch. Further feedback & advice would be very welcome

Happy to post an intermediate result of my attempt to speed this up.
What I did until now:
Primarily I switched from a pessimistic to an optimistic approach regarding the integrity.
This means I've changed the code on several locations to do an integrity check only if the config is flagged as dirty. This ensures that a fixed rule will resurrected and increases the performance massively.
On the other hand I've added a cron job to check the integrity of all configs.
Further, if the evaluation of a rule fails and it throws a RulesEvaluationException, the rule is flagged as dirty now.
With these changes the overall generation of the earlier mentioned 1375 rules takes 42.06 sec now - inclusive the full rebuild of the rules cache at the end of the operation! :)

Open todo's:

• The rules_clear_cache() in RulesPlugin::save() should be replaced. I tried to figure out how to update the cache - but I'm lost in all this caches :).
  For now I simply assume that if is_rebuild is set a full cache rebuild will be done after the rebuild.
• Nice to have: Button to manually trigger the integrity check of all rules

Btw: I would like mention that I actually love the integrity check! It really helps me to build rules programmatically. But I think we can be a bit more optimistic about the integrity of already stored rules.

Update patch:

• Fixed error in the UI code change
• Added button to manually trigger the integrity check of all rules.

Still to do:

• The rules_clear_cache() in RulesPlugin::save() should be replaced
• Check why the test fails and figure out what to do because of this.

Next prio for me has the fact that one specific rules has > 10 Minutes to get be processed (As far as I can see this is related to the dependency detection)
More to come, feedback & advice still very welcome :)

1374 rules on a site? nice!

The rules_clear_cache() in RulesPlugin::save() should be replaced. I tried to figure out how to update the cache - but I'm lost in all this caches :).

In case the saved config is no component, we can skip clearing caches. Only for reaction-rules we have to reset the cache of all involved events... (removed onces too). That should already help a lot.

An idea it might be worth pursuing is to skip the recursive integrity checks of components as done in
'rules_element_invoke_component_validate', but only check for the dirty flag. That would mean, that integrity check fails do not immediately cascade into all used components, but that should be acceptable. Instead we could just check the dirty flag on run-time and bail out there if necessary.

For now I simply assume that if is_rebuild is set a full cache rebuild will be done after the rebuild.

That's the case, so let's go that way and document that in the code.

Primarily I switched from a pessimistic to an optimistic approach regarding the integrity.

Agreed, I was already thinking that too. However, I was more thinking about skipping integrity-checks when clearing caches as this happens regularly when working in the UI, but doing integrity-checks when displaying / after importing rules only.

Further, if the evaluation of a rule fails and it throws a RulesEvaluationException, the rule is flagged as dirty now.

Good idea! That helps us already a lot as that way it doesn't harm much if a dirty rule gets executed as it will be marked as failing anyway. However, I guess we should limit this to ERROR severity to avoid lots of rules automatically failing because of some warnings like empty data values.

+ // Ensure there are no orphaned items.
+ $queue = DrupalQueue::get('rules_cron_integrity_check');
+ $queue->deleteQueue();
+
+ // Add all rules to the queue.
+ foreach (rules_config_load_multiple(FALSE) as $rules_config) {
+ $queue->createItem($rules_config);
+ }

First off, this won't work as you might clear the queue before it's done. Moreover I don't think we should do this as it's going to continuously stress your site with checking rules. We could implement a flag though that tells us we've to run an integrity check and postpone it to the queue? Not sure whether that's needed though as we cannot rely on the queue being done before any rules are getting executed anyways.

Good idea! That helps us already a lot as that way it doesn't harm much if a dirty rule gets executed as it will be marked as failing anyway. However, I guess we should limit this to ERROR severity to avoid lots of rules automatically failing because of some warnings like empty data values.

oh, let's better don't do that. If a rule fails only sometimes it shouldn't automatically go away. Let's run an integrity check if there is a exception of severity 'error' and only mark the rule as dirty if the check fails. (-> update dirty flag).

Qutoes from #13 :

In case the saved config is no component, we can skip clearing caches....

Code extended and documented.

An idea it might be worth pursuing is to skip the recursive integrity checks of components as done in
'rules_element_invoke_component_validate'....

Not sure if I've enough understanding of rules to get this - I've added a piece of code to the patch. Is this what you were think of?

...skipping integrity-checks when clearing caches as this happens regularly when working in the UI, but doing integrity-checks when displaying / after importing rules only....

The patch changes the behaviour of rebuildCache() to only check the integrity, by calling rules_config_update_dirty_flag(), if a rules_config is marked as dirty.
That way a cache rebuild will enable a former dirty rule - but won't check the integrity of clean rules. I'd say that's a nice compromise between performance and integrity.

Further, if the evaluation of a rule fails and it throws a RulesEvaluationException, the rule is flagged as dirty now.

I've added a check for the severity. The flagging was already done by calling rules_config_update_dirty_flag() - thus a rule will be only marked as dirty if it really is.

First off, this won't work as you might clear the queue before it's done...

Entirely removed the cron. If someone likes to check the integrity it can be triggered manually by using the new button on the global rules settings page.

Attached you'll find the export of the two rules I've in my system. If I open admin/config/workflow/rules it takes in total 109 seconds and 107 of them are spent for a call of RulesREactionRule->acces().
I'm wondering if we could add a user based caching (flushed on permission/user update).

+++ b/includes/rules.core.inc
@@ -961,7 +961,13 @@ abstract class RulesPlugin extends RulesExtendable {
+      // In case the saved config is no component, we can skip clearing caches.
+      // If is_rebuild is set, the cache rebuild will be done after the rebuild.
+      if (!empty($this->itemInfo['component']) && empty($this->is_rebuild)) {
+        rules_clear_cache();
+      }

Still, we need to clear the cache of the "event-set" for reaction rules or the changes won't be applied. (e.g. see rules_invoke_event() and RulesEventSet::rebuildCache())

+++ b/rules_admin/rules_admin.inc
@@ -208,10 +208,103 @@ function rules_admin_settings($form, &$form_state) {
+  $form['actions']['start_integrity_check'] = array(
+    '#type' => 'submit',
+    '#value' => t('Run rules integrity check'),
+    '#weight' => 2,
+    '#submit' => array('rules_admin_start_integrity_check'),
+  );

I'm not sure about that. Maybe it should be run full integrity check + clear caches? Also, we should describe it better and explain when to run it.

> An idea it might be worth pursuing is to skip the recursive integrity checks of components as done in
'rules_element_invoke_component_validate'....

Not sure if I've enough understanding of rules to get this - I've added a piece of code to the patch. Is this what you were think of?

Not really. I think we should totally skip the integrityCheck at this point and just trust the dirty flag. However, instead I think we should *always* run the integrityCheck in the UI-overview as it's important to get a real overview and to be able to discover damaged rules.
To make the UI faster though, we should really pursue paging via EFQ. That's already in entity api's admin UI, so we can mostly take over the implementation from there. That's another issue though.

Attached you'll find the export of the two rules I've in my system. If I open admin/config/workflow/rules it takes in total 109 seconds and 107 of them are spent for a call of RulesREactionRule->acces().
I'm wondering if we could add a user based caching (flushed on permission/user update).

Ouch. Instead of creating another layer of complexity I think we should invest the time into optimizing it. I don't think there is anything in there that should be that complex, I suspect some recursive weirdness going on. Decoupling component-action-access from component-access as pursued by #1217128: limiting Rules components to specific permissions should help already help a lot then.

Still, we need to clear the cache of the "event-set" for reaction rules or the changes won't be applied. (e.g. see rules_invoke_event() and RulesEventSet::rebuildCache())

I've added RulesEventSet::save() to flush the cache.

I'm not sure about that. Maybe it should be run full integrity check + clear caches? Also, we should describe it better and explain when to run it.

Added button to clear cache and fieldset with brief hint what these buttons are about. Enhanced integrity check output to include information about the failed rules_configs.

Not really. I think we should totally skip the integrityCheck at this point and just trust the dirty flag.

Changed that way - I hope I missed nothing :)

To make the UI faster though, we should really pursue paging via EFQ

Added paging (it's enabled by default) - this really eases the pain we experience by the access checks.

Fixed issue with tags filter in table overview.

interesting ... following.

Added paging (it's enabled by default) - this really eases the pain we experience by the access checks.

Awesome!

Here a first visual review. Will do a deeper review later on.

+++ b/includes/rules.plugins.inc
@@ -731,4 +734,17 @@ class RulesEventSet extends RulesRuleSet {
+  public function save($name = NULL, $module = 'rules') {
+    $return = parent::save($name, $module);
+    // Ensure cache is rebuild.
+    rules_clear_cache();
+
+    return $return;
+  }

Oh, Event sets are not saved to the database like regular components. They are there for caching purposes only. I guess this should be mentioned somewhere there in the comments.. :/

So best save() should be overwritten to do nothing I guess. For clearing event-specific caches we could add a method to the event-set class.
I fear that for updateing the cache of a given event, we'd have to replicate the logic of the event-set's rebuildCache() method and directly update the cache. Alternatively we could improve the cache rebuilding logic such that it can trigger more fine-granular cache-rebuilds, i.e. rebuild not the whole cache but only event-sets. As changing a single reaction rule might influence multiple events anyway, just rebuilding all event-sets should keep the logic simpler anyway, still we can skip rebuilding all the regular cache and component caches.

+++ b/modules/events.inc
@@ -165,4 +165,4 @@ function rules_get_entity_view_modes($name, $var_info) {
- */
\ No newline at end of file
+ */

Missing newline at end of file ;)

+++ b/rules_admin/rules_admin.inc
@@ -236,10 +239,140 @@ function rules_admin_settings($form, &$form_state) {
+    if (count($rules_configs) == 10) {
+      $batch['operations'][] = array('rules_admin_start_integrity_check_batch_worker', array($rules_configs));
+      $rules_configs = array();
+    }

I guess the batch should do an efq-query and then load configs by id/name, so not all entities are loaded every time.

Not sure whether the batch is really needed though. How long does integrity checking take for you given the improvements of taking away recursive checks?

+++ b/ui/ui.controller.inc
@@ -198,7 +207,39 @@ class RulesUIController {
+    $entity_info = entity_get_info('rules_config');
+    $properties = entity_get_all_property_info('rules_config');
+    foreach ($conditions as $key => $value) {
+      if (isset($properties[$key])) {
+        $query->propertyCondition($key, $value);
+      }
+    }

Let's just add them and assume the properties are there, that's what regular entity_load() does too.

RulesEventSet: So best save() should be overwritten to do nothing I guess

RulesEventSet::save() does nothing now.

I fear that for updateing the cache of a given event, we'd have to replicate the logic of the event-set's rebuildCache() ....

erm, what? That's currently to deep in rules for me to understand. :)
I went for a lucky punch and moved the code I added in RulesEventSet::save() before into RulesReactionRule. But I could bet that isn't what you thought. :P

Missing newline at end of file ;)

Nope, it's contrary - the line was missing and I added it ;)

I guess the batch should do an efq-query and then load configs by id/name, so not all entities are loaded every time.

Not sure if I got that. As far as I understand and debugging shows me rules_admin_start_integrity_check() runs once, loads all the rule configs and then the worker is called multiple times. But it just deals with the handed over entities.
However, changed to use efq-query and work with ids.

Let's just add them and assume the properties are there, that's what regular entity_load() does too.

Done.

Attention I've added another change described in this issue: #1433210: Store result of RulesPlugin::availableVariables()

New patch:

• Fixed error with Filter by event. Is there a way for the entity controller to adjust the EFQ query (adding a join & where)? The way I deal now with these filters is to ugly.
• Added Filter by status

Well, this is getting way too big. Let's move separate chunks out. Thus let's handle the admin UI changes over at http://drupal.org/node/1454136 and focus on improving the integrityCheck approach here.

Also, the availableVariables() improvement got already committed, see #1433210: Store result of RulesPlugin::availableVariables()

I'll take a stab on this.

// In case the saved config is no component, we can skip clearing caches.
// If is_rebuild is set, the cache rebuild will be done after the rebuild.

We cannot rely on the rebuild only happening during a cache-clear, as it could be triggered manually too. Also, clearing the rules cache during rebuilding shouldn't be an issue, as saving a rule during rebuild should not trigger the cache to be built.

In order for caches to be only cleared for components we need to update event-dedicated caches manually. Let's just implement a half-cache clear for that, which removes all cached event-sets only and improve cache-rebuilding to handle that case (TODO).

Attached patch improves integrity checks to still recurse down through used components as that's the only way to get reliable errors for rules making use of those components. However, I've improved the approach to use a global-static-cache in rules_config_update_dirty_flag() what ensures that no component is going to be checked twice (The previous approach was broken). This seems to do the job, but doesn't work with the batching approach. Thus I've changed the manual integrity-check to run on a single page request. We really should be able to do that during one request.
@das-peter: I wonder how long the complete integrity check takes for you now?

Related, I've improved this admin UI settings. Still, the admin setting page is rather cluttered now, I think we should move the new stuff to a new "advanced" tab (just like views has basic+advanced settings).

I also verified that everything goes fine if a rule is broken but not yet marked dirty and so gets executed. The approach of running an integrity check on ERRORs seems to work fine. I've looked over the severity of the evaluation errors thrown and did some corrections in order to ensure we are not missing anything.

ok, I've implemented the 'advanced' settings tab and committed this - thanks!

In order for caches to be only cleared for components we need to update event-dedicated caches manually. Let's just implement a half-cache clear for that, which removes all cached event-sets only and improve cache-rebuilding to handle that case (TODO).

Let's deal with that over at #1425652: Event is not invoked if the cache does not exist for it..

This is just awesome - happy to be able to switch back to the official repo soon :) Thank you for your work!

Guys, I know these boards are for more important matters than handing out praise but I just have to say to fago and das-peter 'thank you for the incredible work you have done on this issue'. I have followed this thread and it is inspiring to see Drupal functioning this way especially for such an important module.
Needless to say, your work has made my life so much easier and I'm sure I'm one of many people for whom that is true!