Problem/Motivation

Hook discovery currently happens during container rebuild, and writes to entries in key/value, which then get cached in the cache API. This is partly because hooks were previously written into the container itself.

Now that hooks are discovered separately, I think it might be possible to move discovery to 'just in time' - e.g. the first time a hook is invoked after a cache clear. This would let us do a couple of things:

1. The hook cache would be rebuildable without a container rebuild, so we could drop the key/value store and only use the cache backend.

2. Because the container would be available when hooks are parsed, we could probably use #3486503: Add a file parsing cache collector to replace some uses of FileCache instead of FileCache - this should reduce memory requirements on cache clears because the hit rate for that cache would be much higher.

3. It might help with some aspects of the sorts of race conditions that appear to be showing up in #3592577: Ensure that hook attributes are never parsed from a stale opcache, e.g. we'd be able to put a lock around the hook discovery if we wanted to.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Comments

catch created an issue.

godotislate’s picture

godotislate
he/him
commented

Now that hooks are discovered separately, I think it might be possible to move discovery to 'just in time' - e.g. the first time a hook is invoked after a cache clear.

Could you elaborate on how you envision this being done? Are you thinking of passing the container into ModuleHandler and ThemeManager?

Also, it seems like the all hooks would be discovered on hook_cache_flush invocation right after cache clear?

catch’s picture

catch
he/him
Primary language English
commented

So there are two parts to HookCollectorPass.

One registers the hook classes as services - that would obviously need to stay in the container pass.

The other writes the hook metadata to key/value - this is what I think we could move out.

::collectAllHookImplementations() uses container.modules and the $module.skip_procedural_hook_scan parameters - we ought to be able to pass those into the module/theme services directly rather than the entire container. If we did that work on demand we wouldn't need the collect, temporary container parameter, write steps.

That is as far as I got though.

godotislate’s picture

godotislate
he/him
commented

So even if we loop through all the class files and register services at compile time, then at run time we'd still need to loop through the hook services to figure out what hooks they implement and then order them. I think that means we'd either need access to the container, or to use a service locator. I am wondering though that if we use a service locator (or similar), whether that would make the container size much larger again, since the argument list to the locator would be a long list of hook services.

I'm actually introducing a tag on the hook services in #3481903: Support hooks (Hook attribute) in any registered service for a different purpose, but maybe it'd become useful for this as well.