Problem/Motivation

In most cases a service doesn't need to be instantiated, along with all if its dependencies, in order to determine which hooks it implements. Reflection can be used directly with a hooks class. Discovery should be optimized to not always directly get instantiated services from the container.

Results of discovery can be cached and utilized on production environments. Discovery caches should use fastchained, as these caches will be used for most requests.

Caching should be opt in. Not opting in won't have a significant performance penalty.

Provide documentation and guidance on how to use caches and add a hook_requirements to warn site builders.

Proposed resolution

  • Separate hooks discovery + make results serializable.
  • Update docs
  • Create d.o guide and optimized docs.

User interface changes

New line item always present on Drupal's status page.

API changes

Internal API changes

Data model changes

New serialized cache
New container parameter

Issue fork hux-3276804

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

dpi created an issue. See original summary.

alexpott’s picture

Priority: Normal » Critical

I was reading the current code and the runtime use of reflection on every alter and hook invocation scared me from a performance perspective. Great to see this issue already exists!

This is a critical issue if this module is to get widespread usage / inform future choices Drupal core might make.

alexpott’s picture

Reading the issue summary.

Caching should be opt in. Not opting in won't have a significant performance penalty.

Are you sure about this? Doesn't it depend on how many classes and hooks are using it?

dpi’s picture

@alexpott We're using Hux, alongside many Hooks classes with some decent DI, and to date I'm not seeing significant performance loss while profiling with Blackfire. In fact while testing requests with this patch its often a single-digit millisecond or so slower.

I'm thinking reflection is fast enough, or PHP8 has some kind of optimizations/internal caching around it. In any case I was really hoping to see significant slowdowns without this patch, and inversely gains with this patch, but have not seen real evidence yet.

I hope to be proven wrong!

  • dpi committed c8b0214 on 1.x
    Issue #3276804 by dpi: Discover and cache hook implementations
    
dpi’s picture

Assigned: dpi » Unassigned
Status: Active » Fixed

Committed!

I'm up for re-evaluating default settings if we find performance is significant without this setting turned on enabled.

I plan to periodically profile performance impact with this off, especially as Hux implementations increase over time.

Status: Fixed » Closed (fixed)

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