Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
As I noticed in #2353357: hook_stream_wrappers_alter() should be removed as it is broken since modules are not loaded on demand, stream wrappers are registered before the page cache.
Right now, that means it tries to invoke hooks before the module system is loaded, with that change, it is still loading configuration before the page cache. We should try to get rid of that by moving the private system file path to settings, but it still should be possible to return a cached page without needing public:// or any other stream wrapper.
Status in Drupal 7
In Drupal 7 stream wrappers got registers in _drupal_bootstrap_full() which happens after page caching.
Proposed resolution
Move it to preHandle, after modules are loaded?
Remaining tasks
User interface changes
API changes
| Comment | File | Size | Author |
|---|---|---|---|
| #1 | stream-wrappers-boot-2392433-1.patch | 798 bytes | Berdir |
Comments
Comment #1
BerdirAnd here is a patch, should not conflict with the other issue.
Comment #2
Wim LeersComment #3
Wim LeersAm I right in assuming that if this is green, it's RTBC-worthy, because we already have test coverage?
Comment #4
dawehnerIt would be great, if we profile how much faster this makes page caching.
Documented the Drupal 7 state.
Comment #5
Wim LeersBerdir said this would bring page cache response time down from ~10 ms to ~5 ms. Depends on the machine of course.
Comment #6
BerdirUh, I'm not sure if that can be applied like this here :)
Agreed on profiling/benchmarking, I'll give it a try.
Comment #7
Wim LeersSorry for the confusion; of course I agree we need profiling, I was just relaying the rough numbers you gave me :)
Comment #8
Berdir$ ab -n 1000 -c1 http://d8/
HEAD
Patch
So, not that much of a difference there.
uprofiler gives me a 6% improvement (but the fluctuation is quite high), Kernel::boot() is 1.5 ms faster due to the stream wrapper service construction + register() gone, but other things were slower, but I think that's just fluctuation. The module implements caches are in apcu, so they're pretty fast (and as we've seen, the broken alter hook makes it look faster than it would be if it would work).
If that 1.5ms is actually correct, that would be around 20-25%. But ab is also around a 6% improvement.
With APC classloader, database cache backend and without settings page cache override.
Comment #9
BerdirGreen :)
Comment #10
Wim LeersTests green, profiling shows an improvement, less work done earlier in the bootstrap, hence less work to do on page cache hits, this is a step in making the page cache faster. I don't see a reason not to do this.
Comment #11
dawehnerAdded a related issue #2392787: Move include statements from DrupalKernel::boot() into DrupalKernel::preHandle() which itself also reduces the number a bit.
Comment #12
alexpottCan we register it before we load the modules. Just in case we pursue #1308152: Add stream wrappers to access extension files
Comment #13
catchI don't think module loading would ever use the stream wrapper though? The extension system should know where modules are, not have to go via a stream wrapper.
Comment #14
BerdirCan update this, I don't care.
The goal is to remove the loadAll() call there anyway with #1905334: Only load all modules when a hook gets invoked.
Comment #15
alexpottTalked with @catch and decided that since #1905334: Only load all modules when a hook gets invoked will change this anyway moving is not relevant.
Comment #16
alexpottThis issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 1fd2961 and pushed to 8.0.x. Thanks!
Comment #18
jhedstromI think this caused the
run-test.shscript to start throwing warnings.#2394327: run-tests.sh throwing stream wrapper warnings.