src/EventSubscriber/GenericEventSubscriber.php simply performs a load() on the config entity storage to load reaction rules by event. That could be very expensive if there are many rules configs, because all of them have to be loaded.

Proposed solution: create a cache that holds all reaction rule config IDs per event.

Comments

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Priority: Normal » Major
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Active » Needs work

WIP at https://github.com/fago/rules/pull/428

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

hm, not so sure how the WIP is related?
I think we need to add some code in \Drupal\rules\EventSubscriber\GenericEventSubscriber::onRulesEvent() which takes the build $component object and caches that. Ideally, that code would be re-usable such that we can cache the $component of component configs with as well.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Issue tags: +needs profiling

Oh? I thought we want to cache the config entity IDs to avoid doing a wildcard config entity load in the event subscriber.

We should probably do some profiling here to ensure that the caching has a significant effect.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

I thought we want to cache the config entity IDs to avoid doing a wildcard config entity load in the event subscriber.

Nope, I think this is already fixed in alex' branch - there actually is some nice key value store in place which allows us to query on config efficiently. I think we just need to cache the generated $component object so we do not have to do all the building up of the object based on multiple config objects.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented

What $component object? You mean the expression object that is created from a reaction rule config?

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

No, I mean the component object as provided via RulesUiComponentProviderInterface. This is the central object used by UI + execution, which does not depend on config. But the component used during event execution will be different, as it should add all reaction rules of the event to it. Right now we execute the expressions manually in a state, but if we put it into a component we should be able to cache it nicely, i.e. using the same way for component configs? -> see \Drupal\rules\EventSubscriber\GenericEventSubscriber::onRulesEvent()

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

Maybe we should add an EventSet expression for that? It worked like that in d7 also, i.e. it created an event set object and cached it.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented

Hm, for executing reaction rules we don't need a rules component? That would be just a lot of setup, while we currently just execute the expressions, which is faster? What could we cache in that regard?

In GenericEventSubscriber we do:

$config->getExpression()->executeWithState($state);

How would RulesComponent help with that?

Or do you mean creating a new class EventExpressionExecutor that holds the expression objects? It would not be an expression object, because it is just a container for expressions.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

Or do you mean creating a new class EventExpressionExecutor that holds the expression objects? It would not be an expression object, because it is just a container for expressions.

No, I meant something like an ActionSet - this does nothing more than looping over expressions and executing them either but can hold all that code related to logging or cloning state. I don't see any harm in doing an expression plugin for that?

>How would RulesComponent help with that?
Good question. Setting up the state is something that might be good to cache, but then this should be straight-forward and cached by the EventManager. So I guess what it would end-up caching mostly is the created expression objects, so creating the expression objects from configuration arrays would be cached - instead we've a serialized tree of objects which is ready to execute. Then, of course - one cache hit is better than many ;)

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

But yeah, we definitely should start benchmarking this - e.g. make a page request which runs a rule and fire ab on it. Or just run it a PHP-loop, but that would benefit from static-caches in an unrealistic way.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented

Did some profiling of the current 8.x-3.x branch.

Generating dummy Rules with this script (warning: deletes all your Rules!):

$nr_rules = 10;

use Drupal\rules\Context\ContextConfig;

$expression_manager = Drupal::service('plugin.manager.rules_expression');
$storage = Drupal::service('entity_type.manager')->getStorage('rules_reaction_rule');

// Delete all rules first.
$storage->delete($storage->loadMultiple());

for ($i = 0; $i < $nr_rules; $i++) {
  $rule = $expression_manager->createRule();

  $rule->addAction('rules_system_message', ContextConfig::create()
    ->setValue('message', $i)
    ->setValue('type', 'status')
  );

  $config_entity = $storage->create([
    'id' => "test_rule$i",
    'events' => [['event_name' => 'rules_entity_view:node']],
  ])->setExpression($rule);
  $config_entity->save();
}

Then adding the following to settings.php to disable render caching so that the rules fire on every node page request:

$settings['cache']['bins']['render'] = 'cache.backend.null';
$settings['container_yamls'][] = DRUPAL_ROOT . '/sites/development.services.yml';

Create a node so that there is a page that we can make requests to.

Then run the generate scripts with different values for $nr_rules.

To get some numbers I use Apache bench:

ab -n 20 http://drupal-8.localhost/node/1

That sends 20 requests in serial to Drupal.
The median response time is then Percentage of the requests served within 50% of requests.

Results on my laptop with this:
Median:
1 rule: 392ms
10 rules: 412ms
100 rules: 491ms
1000 rules: 1572ms

Note that generating those 1000 rules already takes quite a while :-D

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented

Meh, I had Xdebug enabled - disabled that and repeated the test:

1 rule: 272ms
10 rules: 274ms
100 rules: 318ms
1000 rules: 1014ms

But really the most surprising number is when I uninstall Rules: 92ms

Not sure what is going on there - probably some cache kicking in that I have overlooked.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented

Tested again with this hook implementation added to user module that does the same thing as the show message action in the rule:

function user_entity_view(array &$build, EntityInterface $entity, EntityViewDisplayInterface $display, $view_mode) {
  if ($entity->getEntityTypeId() === 'node') {
    drupal_set_message('a');
  }
}

Median for that is 248ms, which sounds more realistic.

Rules adds a performance penalty of 10% when enabled with one rule compared to a developer implementing the same functionality in a hook.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago at drunomics commented
Status: Needs work » Needs review

I got everything working so far, but it doesn't work because serialziation of rule objects is broken. I'll fix that in a separate issue (separate problem), so I think this is ready for a first code review.

https://github.com/fago/rules/pull/447

The architecture allows modules implementing different config-storage to add their own component resolvers (e.g. for storing rules in content entities). Also, the RulesComponentRepository which handles the caching logic is a service which could be swapped by a module / future PR that wants to implement a PHP-code execution strategy. So the approach should be future-safe.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs review » Needs work

looks good and tests are passing except for coding standards, setting to needs work for that.

we should also repeat the profiling to check how much has improved.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago at drunomics commented
Status: Needs work » Needs review

Thanks, I corrected the coding style issues. Could you help me with the profiling and re-run your test so we see what changed?

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago at drunomics commented

ok I repeated the tests:

Test (without xdebug):
ab -n 20 http://drupal-8.localhost/node/1

Without rules:
Time per request: 164.178 [ms] (mean)

Without the patch:
0 rules: Time per request: 164.495 [ms] (mean)
1 rule: Time per request: 172.297 [ms] (mean)
10 rules: Time per request: 175.394 [ms] (mean)
100 rules: Time per request: 220.783 [ms] (mean)
1000 rules: Time per request: 699.044 [ms] (mean)

With the patch:
0 rules: Time per request: 155.587 [ms] (mean)
1 rule: Time per request: 161.042 [ms] (mean)
10 rules: Time per request: 163.648 [ms] (mean)
100 rules: Time per request: 190.642 [ms] (mean)
1000 rules: Time per request: 580.814 [ms] (mean)

Generally, the tests had high varying results when I repeated them (+-30ms!). I just tried to ran them ~10 times each and picked the lowest numbers I got. Generally I was able to get lower numbers with the patch applied though. Else we see executing the Rules is quite performance costly and would need more profiling & improvement.

  • fago committed e8f6a91 on 8.x-3.x 
    Issue #2513076 by fago: Add event amd component caching when rules are...
fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago at drunomics commented
Status: Needs review » Fixed

ok, merged it.

Status: Fixed » Closed (fixed)

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