Integration of rules_optimization with rules 2.x

Great to see you already started!

>And this will be integrated in Rules2.x after separate development.
We'll see, but if the solution is simple&fast, why not.

>Currently it uses ECA (event-condition-action) strategy. For a large rule sets this strategy is not optimized one.
This is not true. ECA is fine, but there is potential to optimize the evaluation strategy of rule sets in order to minimize the number of evaluated conditions.

>For the evaluation execute function is called by default by rules_invoke_rule_set. If this scheme is used for every rule set the execution can be override in hook_rules_plugin_info. But if this is not the case, i.e. selective modules are using optimization it will make trouble.

I don't see what you mean here? Well, what your patch misses is the actual $set->optimize(); call before the rule set is cached -> that's happening in the rebuildCache() method such that the implementation is actually invoked.

beside that:
* As discussed we should start with the event set only to avoid having conditions that depend on the outcome of actions. Your code though adds the method only to ususal rule sets, but not to the RulesEventSet.
* The method of the interface misses a doxygen comment. Also the doxygen of the interface should describe how it is supposed to work / when its called / what is expected to do. The interface itself could be named a bit better, I'd suggest RulesOptimizationInterface.
* The default extender class needs to reflect it's the default implementation in the name and a comment describing that.

@git:
I love using git for the patch workflow, but it has the downside that it's not integrated with the automated tests yet. So I'm fine with using git for patch development, but let's also post the patches in the issue queue such that the test bot works.

subscribing, no patch here -> so it's active.

* The patch seems to undo some latest changes. Always have a look at your patches to check everything is as you want before submitting them.
* Make sure the testing bot gives you a green light.

+++ rules/includes/rules.plugins.inc
@@ -569,9 +569,7 @@ class RulesEventSet extends RulesRuleSet {
         $event->name = $name;
-        foreach ($rules as $rule) {
-          $event->rule($rule);
-        }

was that change intended?

+++ rules/includes/rules.plugins.inc
@@ -589,3 +587,23 @@ class RulesEventSet extends RulesRuleSet {
+   * Convert the input rules into a decision tree structure
+   * ¶
+   * @param $rules
+   *   All the rules trigerrable by the RulesRuleSet or RulesEventSet  ¶
+   */

trailing whitespaces

+++ rules/includes/rules.plugins.inc
@@ -589,3 +587,23 @@ class RulesEventSet extends RulesRuleSet {
+  public function optimize($rules){}
+}
\ No newline at end of file

there should be a newline at the end of the file

Powered by Dreditor.

But the goal of this patch should be a working Rules module that just allows optimization. So Rules should stay fully functional with this patch and it should be possible for your new module to easily extend/implement an optimization technique.

+++ rules/includes/rules.plugins.inc
@@ -570,6 +570,7 @@ class RulesEventSet extends RulesRuleSet {
+          $rule = $event->__call('optimize', $rule);

you want to optimize for each rule? I think one optimze() call to the whole event set should be enough.

+++ rules/includes/rules.plugins.inc
@@ -589,3 +590,30 @@ class RulesEventSet extends RulesRuleSet {
+   */
+  public function optimize($rule);
+}

what do you need the single rule in this case for?

Generally I would recommend that you run the Rules tests on your local Drupal installation before you submit a patch here. You can check failing tests and you can avoid the red light from the bot. Your current patch produces fatal errors.

Powered by Dreditor.

obviously testbot is broken, see also another issue http://drupal.org/node/649438#comment-3078996

+++ rules/includes/rules.plugins.inc
@@ -589,3 +590,28 @@ class RulesEventSet extends RulesRuleSet {
+/**
+ * Interface for optimized rules
+ */
+interface RulesOptimizationInterface {
+  /**
+   * Convert the input rules into a decision tree structure
+   *
+   * @param $rules
+   *   All the rules trigerrable by the RulesRuleSet or RulesEventSet

the comments need some fixing - I would suggest "Interface to allow third party modules that optimize rules evaluation."

and for the function:
"Rearranges the internal structure of rules to optimize evaluation". The decision tree will be a technique that your module will apply, so a similar comment should go to your module. The interface here allows any generic optimization method that we may not even no about yet.

+++ rules/includes/rules.plugins.inc
@@ -589,3 +590,28 @@ class RulesEventSet extends RulesRuleSet {
+/**
+ * Class implementing optimization method
+ */
+class RulesOptimization extends FacesExtender implements RulesOptimizationInterface {

"Class implementing a default optimization method that is empty."

+++ rules/includes/rules.plugins.inc
@@ -589,3 +590,28 @@ class RulesEventSet extends RulesRuleSet {
+  /**
+   * Implementation of this method would be in
+   * rules_optimization module.
+   * After that it would be integrated
+   */
+  public function optimize($rules){}

"Empty default implementation" - or something similar. Do not mention your module here, as any other module could do the optimization as well. Rules itself does not need to know which module actually optimizes.

Powered by Dreditor.

+++ rules/includes/rules.plugins.inc
@@ -589,3 +590,28 @@ class RulesEventSet extends RulesRuleSet {
+/**
+ * Interface to allow third party modules
+ * that optimize rules evaluation
+ */

why don't you use more of the space in one comment line and break so early?

+++ rules/includes/rules.plugins.inc
@@ -589,3 +590,28 @@ class RulesEventSet extends RulesRuleSet {
+  /**
+   * Rearranges the internal structure of rules
+   * to optimize evaluation
+   *

same here

+++ rules/includes/rules.plugins.inc
@@ -589,3 +590,28 @@ class RulesEventSet extends RulesRuleSet {
+ * Class implementing a default optimization method that is empty

sentences in comments should end with a .

Powered by Dreditor.

+++ rules/includes/rules.plugins.inc
@@ -570,3 +571,26 @@ class RulesEventSet extends RulesRuleSet {
+/**
+ * Interface to allow third party modules that optimize rules evaluation.
+ */

"Interface to allow third party modules to optimize rules evaluation." sounds better

+++ rules/includes/rules.plugins.inc
@@ -570,3 +571,26 @@ class RulesEventSet extends RulesRuleSet {
+   *   All the rules trigerrable by the RulesRuleSet or RulesEventSet.

should be "triggerable"

Powered by Dreditor.

This are just some nitpicks, I leave the final review to fago.

The RulesOptimizationInterface should be in the rules.core.inc somewhere near to the other interfaces there as it is not actually implementing a plugin.

>+ * Empty default implementation.
I'd call it not just empty, but say it does nothing. The default class RulesOptimization should be better named to reflect its the default behavior, e.g. use RulesDefaultOptimizer.

>+ public function optimize($rules){}
Nitpicking, but there should be space before {}.

Then the $rules parameter should be removed. First off the faces extension mechanism makes the extended object (in our case the RulesEventSet) available, so are the rules as they are children of it. Secondly, the optimization interface is with $rules bound to optimize RuleSets, without it it could be theoretically used for optimizing any plugin.

+++ rules/includes/rules.plugins.inc
@@ -551,6 +551,7 @@ class RulesEventSet extends RulesRuleSet {
+        $event->__call('optimize', $rules);

$rules needs to be removed.

Powered by Dreditor.

#38: rules_optimization_integration.patch queued for re-testing.

#38: rules_optimization_integration.patch queued for re-testing.

As the testbot seems to be not interested to test this finally, I mark it as RTBC as it looks good to me.

Agreed. I wrote some additional docs, moved RulesDefaultOptimizer to rules.core.inc (plugins is only for plugins) and ran the tests myself. All fine -> committed, thanks.