Performance optimization: dynamic event subscription

Giving this a higher priority as we got asked about this quite a lot recently. So, we should do some more research on this to see if there is a reliable approach to this optimization. Maybe something like caching the list of used events in ECA config models, forcing a container refresh afterwards to rebuild ECA's event subscribers based on that list?

Rebuilding the service container is the reason why Rules is currently not reliable, because event subscribers are being defined in a moment where not all services are completely declared yet. For a reliable approach, we need to look for a different way without the need of a container rebuild.

Quite recently I had an idea how to fix that for Rules using a lazy event subscriber. The current patch can be found here: https://www.drupal.org/files/issues/2023-08-02/2816033-lazy-subscriber-w... → (from https://www.drupal.org/project/rules/issues/2816033#comment-15175990 🐛 Rules registers no listeners on rare occasions. Needs review ).

For ECA the idea could be to implement the lazy event subscriber in the way that it reads the subscribed events from cache.backend.chainedfast and if a cached list is not available, it builds up the list like it's already happening in the EcaStorage::loadByEvent method and then caches it. The lazy event subscriber needs to be added to the event_dispatcher service via service provider as can be seen in the linked patch file.

That approach may only work when there's not variation in the event subscriber implementation, because that lazy subscriber is then the only one triggering for the subscribed events. Could be a conflict with 📌 Make all token data providers discoverable and expose them in the UI and in the ECA Guide Active if you wanted to move token data declarations to the event subscribers.

Would that also mean that all the ECA integration modules would not require any EventSubscribers any longer?

Could be a conflict with #3348422: Move provided tokens in EventSubscribers to events implementing DataProviderInterface if you wanted to move token data declarations to the event subscribers.

We can hold back on that one if we go for the proposed optimization first. Sounds like we should then move the DataProviderInterface to the ECA event plugins as the only remaining commonality, right?

Would that also mean that all the ECA integration modules would not require any EventSubscribers any longer?

Probably yes. Could also consider making it optional, in case an integration can't get around providing its own event subscriber for whatever reason.

Sounds like we should then move the DataProviderInterface to the ECA event plugins as the only remaining commonality, right?

Probably yes, in case the plugin instance is already available at that point. Haven't checked the processor implementation in detail. At first thought the plugin sounds like a proper place. Thinking this further, if DataProviderInterface would also have a declarative method (telling the system what tokens it provides), then this approach might also be suitable for action plugins.

Probably yes, in case the plugin instance is already available at that point.

They must be, the processor relies on the plugins entirely.

Thinking this further, if DataProviderInterface would also have a declarative method (telling the system what tokens it provides), then this approach might also be suitable for action plugins.

As for action plugins, it's more difficult, as actions are not ECA specific. But those, who are from other modules (or core) won't ever define any tokens in the first place.

As for event plugins, it seems to make even more sense, as then we would have everything about the ECA event in that plugin. Sounds like a better DX as well.

@mxh any chance for you to work on the lazy event subscriber? Would be nice to get this done first so that we can then start building the other components.

any chance for you to work on the lazy event subscriber?

Sorry, I'm not available. You can have a look at the linked patch in #7 how it could be implemented.

This is now implemented and working. Modules no longer have to subscribe to all their events, ECA takes care of that with a lazy and dynamic event subscription for those that are used in any active model.

Event subscribers of submodules are already removed, unless they also subscribe to other events like EcaEvents::BEFORE_INITIAL_EXECUTION. That's why relatively many files got changed in the MR.

Also, if an event has a non-default priority, this is now being declared in the ECA event plugin. Example: \Drupal\eca_config\Plugin\ECA\Event\ConfigEvent::definitions for the event import_validate.

Setting to NW due to failing tests.

Took a brief look into the MR and found a couple of problems:

• Using Drupal's state service is not ideal for storing the list of events, because it adds a second source of truth besides the ECA configurations. Plus there is a potential concurrency problem when multiple users save ECA condigurations (one can overwrite the other's state value). The patch I linked in #7 is just using state because Rules already uses it. For performance and integrity, I highly recommend to use a regular caching approach instead, as described in #7 by making use of cache.backend.chainedfast. That caching approach doesn't require any upgrade path, while using state would require an upgrade path due to the missing state value for existing sites when switching from 1.x to 2.x.
• The MR currently doesn't allow an integration module to provide its own event subscriber, or at least a way to add its own logic for event subscription. For example, the eca_misc module needs its own logic for facading event objects. In the MR you moved logic of that particular module up into the src/EventSubscriber/EcaBase class, where it clearly does not belong to as it breaks stable dependency principle (eca is basically using classes of eca_misc). We need a way that integration modules like eca_misc are still able to include their own logic.
• The LazySubscriber should not instantiate a class called EcaBase, because base classes are usually abstract classes.

When this API change comes in, it needs to be at least made sure, that even when existing integration modules still do it the old way, that events are not being subscribed more than once (like the lazy subscriber is subscribed plus the integration module subscribed). This could be done for example by removing/renaming the src/EventSubscriber/EcaBase class so integration modules are forced to change their integration logic. Or we provide a way so that both the old subscriber mechanic still works and the new lazy approach, for example by introducing another option in the event plugin definition.

As for tests: I was already working on it, and they're now fixed. The logic behind event subscriber priority wasn't working correctly yet and the config import validator was the only one that used a non-default priority.

As for state vs. cache: I tried to use cache for a long time and always rand into a circular dependency injection issue. With using state instead, that problem went away. An upgrade path would be simple though, but of course necessary.

As for prepareEvent: this mechanic is only used by eca_misc and nowhere else. That looks to me as if this is a "feature" that's not needed and should be deprecated. And as eca_misc is packaged with eca, it is safe to handle this special case that way. The classes will only ever be used when eca_misc is enabled, so nothing currently breaks with this approach.

If the event preparation should be brought back, then we should see if this could be moved into the ECA Event plugin.

As for LazySubscriber instantiating EcaBase: a lot of base classes in the Drupal eco-system are not abstract and that's why I continued to use that. But we can easily rename that class, if that makes any difference. All modules implementing with ECA will have to upgrade already, so this would then be an enforcement for them to either remove or update their EventSubscriber, if they had one. That addresses also the last paragraph in #15. But even if we kept the current EcaBase, a potential double subscription (which we can never prevent) would not lead to a double ECA processing, since I've removed the processing code from the onEvent method.

I leave the status for this on NW since there is certainly more work to be done. Some of the todos are:

• Provide update path if we stay with the state approach; otherwise find a safe way to use the cache backend.
• Rename the base event subscriber and update the remaining subscribers in sub-modules.
• Decide about event preparation: feature required (move it to event plugin) or can be removed and the single case treated as a special case?
• Clean up usage of \Drupal\Component\EventDispatcher\Event|\Symfony\Contracts\EventDispatcher\Event. In D10 this is now simply \Symfony\Contracts\EventDispatcher\Event

Working on this for a bit.

I tried to use cache for a long time and always rand into a circular dependency injection issue.

Can confirm this, that's a bummer. Using state seems to be the only viable option for now.

Continued on the MR in one commit (see above), feel free to revert it if you don't want it to be there.

Changes I've applied there:

1. Removed EcaBase completely to enforce existing integrations making use of the new API. Think it's better to produce a fatal to let them know that the API needs to be adapted instead of silent failures (which would mean no execution at all).
2. Removed the lazy event subscriber, because looking at the event listener's logic in ECA for a second time - it doesn't make sense to provide two layers of event subscribers here. Instead I renamed it to be a "dynamic event subscriber" which holds a "lazy-like" invokation of the processor's execution logic when needed.
3. Processor and EcaEvent objects are now making use of EcaEvent::execute method. With that mechanic, the system (Symfony) event object can be passed to the event plugin instance. And when that is available from within the event plugin, the event plugin is able to work with that object, for example to provide token data (may be relevant for 📌 Make all token data providers discoverable and expose them in the UI and in the ECA Guide Active )
4. Created a new base class for ECA execution-related events. It is likely that most of them may be removed anyway in the future once token data providers are completely handled by the according event plugins. But for this change here it may be fine.
5. Provided an upgrade path that writes the state value by implementing an update hook.
6. Added lock mechanic to minimize a potential concurrency problem when writing into state, implemented within Drupal\eca\Entity\EcaStorage::resetEventSubscriber.
7. Removed the facade classes of eca_misc entirely and moved their contained logic into the event plugin implementation, which is solely about providing token data. For now we have no alternative for event preparation, but as argumented in #16 this should be fine. Also now that we enable more involvement of the event plugin, we might not need such mechanic at all anymore.

I think with these changes, all points in #16 were addressed, with exception of

Clean up usage of \Drupal\Component\EventDispatcher\Event|\Symfony\Contracts\EventDispatcher\Event. In D10 this is now simply \Symfony\Contracts\EventDispatcher\Event

IMO this would add even more changes, and it's worth to be addressed as follow-up: 📌 Cleanup usage of Event classes Active

Further notes:

When introducing this change, we'll have 3 initialization layers:

1. The event subscriber that reads from state
2. The ECA entity storage that reads from cache (EcaStorage::$configByEvents=
3. The processor service that loads all event objects from an ECA config: foreach ($eca->getUsedEvents() as $ecaEvent) {

Think this initialization process can be further optimized. For example, it may be considered to just put in all initialization-relevant values into one array and put it into state. That might reduce some I/O (point 1 and 2) and object instantiation overhead (point 3).

This is a fantastic piece of work, thanks @mxh. I've reviewed this and everything looks as it should be.

One minor thing, though, that could be reviewed: the update hook. I wonder if we should enhance \Drupal\eca\Entity\EcaStorage::resetEventSubscriber such that without providing arguments, this method would do its work even without comparing $orgEvents !== $newEvents.

That way we would get a chance for a state refresh, which should be available not only for the update hook. According to Tim Plunkett, the state service should only ever be used for reproducible content. This is the case for our implementation, but I think it then also needs a method to actually re-create the state by a simple method. So, it could simplify the update hook, and we could also provide a drush command for that. I'd be happy to provide the code for that, just wanted to check first, what you think.

Just committed a couple of code style clean-ups.

Also, the last commit contains the suggested state reset command.

I wonder if we should enhance \Drupal\eca\Entity\EcaStorage::resetEventSubscriber such that without providing arguments, this method would do its work even without comparing $orgEvents !== $newEvents.

Don't see any use for that, except for error situations (which should hopefully never happen). But yeah why not, like you already implemented should be fine.

Really wondering whether we could address the three points raised in #18. Especially regarding point 3 of #18, I don't like that all event objects of an ECA configuration entity need to be instantiated so that you know whether it is even relevant for the particular event. Seems like another resource overhead that may be worth to be optimized. But could also be addressed in another follow-up.

Is number 3 in #18 about the ECA Event Plugin, that we should also remember? That would then allow us to just instantiate that one to do all the extra stuff when applicable, right? If I'm not wrong, this would be done by extending the array which gets stored in state. I'd be happy either way, either addressing this here or in a spearate issue.

Sorry, there are two numbered lists in that comment. I meant following:

The processor service that loads all event objects from an ECA config: foreach ($eca->getUsedEvents() as $ecaEvent) {

Think we can just address it separately.

Ah, missed that. Yes, let's handle this separately. Marking this as RTBC now and will be merging this.

Merged the MR and started a Change Record at https://www.drupal.org/node/3381190 → where we should also document the changes so that other developers can update their modules accordingly.

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