Don't dispatch events if there are no rules that use them (esp. business_rules_entity_load)

Just ran some profiling on this, has no rule using the entity load event, and the node edit pages timeline took 7628ms, after return directly in business_rules_entity_load, the same node edit page only took 1940ms to open.

From profiler, nearly 1500ms was spent in business_rules_entity_load.

Attach a patch to temporarily fix this if the event_load event is not needed, until there is a better solution.

Events are getting dispatched even though no Business Rule is set. Added patch to add the check if the Business rule has been added or not for the entity before dispatching the event.

After applying the #5 I can say that the Events are not getting dispatched when there is no Rules setup. It's working fine for all other cases too. businessRulesExists() is serving the purpose.

Also, I think it should be marked as Bug Report, not a feature request.

+++ b/business_rules.module
@@ -346,52 +346,58 @@ function business_rules_entity_load(array $entities, $entity_type_id) {
+    $event          = new BusinessRulesEvent($entity, [

Too much whitespace before the "=". It doesn't need to line up with the previous line.

+++ b/business_rules.module
@@ -529,3 +535,29 @@ function business_rules_entity_form_display_alter(EntityFormDisplayInterface &$f
+/**
+ * Check if Business Rule exist for the entity or not.
+ *
+ * @param string $reacts_on
+ *   The Event Name.
+ * @param \Drupal\Core\Entity\EntityInterface $entity
+ *   The Entity.
+ * @return bool
+ *   Business Rule Exists for entity or not.
+ *
+ * @throws \Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException
+ * @throws \Drupal\Component\Plugin\Exception\PluginNotFoundException
+ */
+function businessRulesExists($reacts_on, $entity) {
+  $business_rule_entity = \Drupal::entityTypeManager()
...
+      [
+       'reacts_on' => $reacts_on,
+        'target_entity_type' => $entity->getEntityTypeId(),
+        'target_bundle' => $entity->bundle(),
+      ]
+    );
+  return !empty($business_rule_entity);
+}

The idea makes sense, but let's not add any more procedural code to the module file. It's already long enough. Let's do this in OO, ideally via an existing or new service.

@colan Thanks for the review. I will update patch in sometime.

@colon, I have updated the patch as per your review. Though I feel lots of coding standard issues are there in the BusinessRulesProcessor.php. I think it can be addressed separately.

+RTBC, #11 LGTM..applied patch and it stopping unnecessary dispatching events.
Just Note: Might we can do in separately: if ($entity instanceof ContentEntityInterface && \Drupal::service('business_rules.processor')->businessRulesExists('entity_presave', $entity)) it exceeds 80 and difficult to read, might we can divide in coding standrards issue.

Looks good. Just some minor things to fix.

1.

+++ b/business_rules.module
@@ -45,13 +44,13 @@ function business_rules_help($route_name, RouteMatchInterface $route_match) {
-  if ($entity instanceof ContentEntityInterface) {
+  if ($entity instanceof ContentEntityInterface && \Drupal::service('business_rules.processor')->businessRulesExists('entity_presave', $entity)) {

Instead of duplicating both checks everywhere, can we put the instanceof check at the beginning of the exists() method? We would only need to rely on that.

Something like:

if (!($entity instanceof ContentEntityInterface)) {
  return FALSE;
}
...

2.

+++ b/src/Util/BusinessRulesProcessor.php
@@ -705,6 +714,33 @@ class BusinessRulesProcessor {
+   * Check if Business Rule exist for the entity or not.

Grammar: Should be "Check if a Business Rule exists for the entity or not."

3.

+++ b/src/Util/BusinessRulesProcessor.php
@@ -705,6 +714,33 @@ class BusinessRulesProcessor {
+   *   Business Rule Exists for entity or not.

But what's being returned? This would be clearer: "TRUE if the business rule exists; FALSE otherwise."

4.

+++ b/src/Util/BusinessRulesProcessor.php
@@ -705,6 +714,33 @@ class BusinessRulesProcessor {
+  public function businessRulesExists($reacts_on, EntityInterface $entity) {

Let's rename this to ruleExists() to make it clear that we're not talking about the module itself, whether it's installed or not. And we don't need to mention "Business" as that's already in the service/class name.

@colan thanks for the suggestions. I have made changes as per suggestions in the comment #15.

Code looks good; thanks! Let's just wait for someone to test it, RTBC it, and then I can commit it.

LGTM.
yeah checking $entity instanceof ContentEntityInterface in ruleExists, Make sense

Thanks!

I'm wondering if I'm seeing a related issue and if anyone has any advice. I'm currently profiling and load testing a site that is heavily using business_rules, and in NewRelic I'm seeing very poor page load performance on all pages, and it seems like business_rules is the cause of much of the slowness.

I'm attaching a sample NewRelic trace for a very simple page, where it's showing BusinessRulesProcessor::process taking 2.5 seconds of the page request.

Granted there are other issues (Twig template) that I've yet to dig into, but business_rules is the main contributor. Anyone know if the proposed patches here will help resolve some of this slowness, or is what I'm seeing a separate issue?

I eventually will patch the module on my local env to see if I see any improvement (without the benefit of NewRelic profiling), but just thought I'd ask if others have seen this issue and/or if i should file a separate issue.

Thanks!

Anyone know if the proposed patches here will help resolve some of this slowness, or is what I'm seeing a separate issue?

It's not proposed; it's been committed to the development branch, and will be included in the next release (unless it's causing serious problems that would require a reversion). If you switch your Dev or Staging sites to use the dev branch, 8.x-1.x, you'll have the new code. And if you can run New Relic on one of those, even better.

The only way to tell if it's this particular issue you're running into, and not another performance issue with Business Rules (for example), would be to try the new code. But it's likely that this is it as there are no other ones reported that I'm aware of.

The only way to tell if it's this particular issue you're running into, and not another performance issue with Business Rules (for example), would be to try the new code. But it's likely that this is it as there are no other ones reported that I'm aware of.

Thanks @colan ! Much appreciated. Indeed I am switching to the dev branch of business_rules now and will run additional tests. I just did an informal "smoke test" and indeed performance looks a little better.

👍

Thought I'd follow up and confirm that this patch (in the dev branch) has definitely improved performance for the site I was testing. Overall page response time went down considerably and I am no longer seeing slow transactions in NewRelic related to BusinessRulesProcessor::process, whereas previously I was seeing these slow transactions even on pages that didn't have business rules on them.

Thanks again!
Brian