Performance optimization: Unify the event and process initialization layer

Created on 16 August 2023, over 1 year ago
Updated 18 August 2023, over 1 year ago

Problem/Motivation

Follow-up from Performance optimization: dynamic event subscription Fixed :

We now 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).

Drupal's state service is already using a static in-memory cache once a value got loaded. That means that we can already make use of that service's caching here.

This optimization will include some heavy changes, also on the processor, which needs to be thoroughly tested.

When addressing this, it can be validated whether we can reduce the event's complexity regards its mostly redundant logic that is put in to the applies and appliesForLazyLoadingWildcard method. If done right, we only need one method.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

📌 Task
Status

Fixed

Version

2.0

Component

Code

Created by

🇩🇪Germany mxh Offenburg

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @mxh
  • 🇩🇪Germany mxh Offenburg
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.1.x + Environment: PHP 8.1 & MySQL 8
    last update over 1 year ago
    297 pass
  • @mxh opened merge request.
  • Status changed to Needs review over 1 year ago
  • 🇩🇪Germany mxh Offenburg

    Created a first MR draft that unifies the three initialization layers mentioned in the IS, by using one state value. Within that unification refactor, I renamed some (newly introduced) methods and commands, as it seems to make more sense being named that way.

    It also seems that we can get rid of the event's applies method entirely, and solely rely on the lazy event appliance check. Of course this is a major API change that requires any integration to adapt their event classes. However that will happen anyway when we plan to move such logic into the event plugin implementation instead. Probably makes sense to see this issue as a blocker for the event plugin refactoring when we want to make this change.

    The EcaStorage class is now way simpler and does not contain any event initialization logic, this is now completely covered by the Processor, where it belongs to. Also removed the memory cache implementation for event objects from the Eca configuration entity, because it doesn't make sense to cache them when looking into the code parts that invoke /ask for ECA event objects. This reduces a bit of I/O overhead because no cache tags are involved anymore for those objects.

  • Issue was unassigned.
  • 🇩🇪Germany mxh Offenburg
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.1.x + Environment: PHP 8.1 & MySQL 8
    last update over 1 year ago
    297 pass
  • First commit to issue fork.
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.1.x + Environment: PHP 8.1 & MySQL 8
    last update over 1 year ago
    297 pass
  • Status changed to RTBC over 1 year ago
  • 🇩🇪Germany jurgenhaas Gottmadingen

    This is astonishing! I like this approach a lot, and I've not figured a single issue by reviewing this. Only a couple of code style minors, that I've fixed and committed in a single commit.

    Yes, testing is required. I tend to suggest that we merge the MR and tackle potential issues in follow-up issues separately, simply because that makes testing much easier, also fort other folks hanging around. What do you think?

  • 🇩🇪Germany mxh Offenburg

    That's great to hear, and thanks for looking into this. I'd also vote for merging this. Then also further refactoring can be addressed, such as moving ECA-specific logic like ConditionalApplianceInterface and others completely into the according ECA plugin implementation.

  • Open in Jenkins → Open on Drupal.org →
    Core: 10.1.x + Environment: PHP 8.1 & MySQL 8
    last update over 1 year ago
    297 pass
  • Status changed to Fixed over 1 year ago
  • 🇩🇪Germany jurgenhaas Gottmadingen

    OK, merged this and added the issue to the CR for later detailed explanation of what module maintainers need to update in their modules.

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

Production build 0.71.5 2024