This is becoming more relevant with the ongoing change to deprecate and remove text_with_summary from core, for which we'll need a more generic solution.
The interface is an interesting idea, but it should be adopted by core as well for text_with_summary, which would make the dependency issue apparent. This would need to live in a core namespace I think.
I had an idea in https://git.drupalcode.org/project/drupal/-/merge_requests/12897#note_60... to use the existing property settings to find those properties, but looping through all property definitions of all fields would be quite expensive, so we'd still need something to identify field types that would also cover non text based types, to deal with the original idea and the faq field example.
It doesn't look like #70 was considered so far? This targets 7.x now and adding a parameter to an exist method on an interface is still a breaking change.
There are means to do in a BC way now with commented properties and stuff, but I'm still fairly certain that this is not required to pass $options through because we can apply options to the returned URL object?
Also seems to need a rebase.
The cache lookup reductions do look nice, to be fair, that's only going to be true for for one language though. I see we have no performance tests yet when accessing a different language, I don't remember if we discussed that before, but that seems like a useful thing to have? In a different issue of course?
No, it does not, see earlier comments. If it does, then you are using patches for linkit.
This is just a copy paste from the order entity change, yes. I suppose this could say that customer profile pages are not expected to be viewed by anonymous users or something like that.
Thanks bot, forgot to revert the test file again.
Looking at https://git.drupalcode.org/issue/drupal-3539919/-/jobs/6807564, it doesn't seem to run any tests, it also doesn't list the changed tests as changed files, so you might need to set up a separate MR. or we'll just have to verify locally, I'll try doing that when I find some time.
Thanks, I think this is ready then. fairly straightforward and another .inc file dealt with.
I think you forgot to add the class to the MR, but yes, something like that is what I was thinking. To account for external fibers that we don't control and that aren't likely to implement this wait-for-more pattern I would probably invert it. While by number of suspends, this pattern will be more common, I would expect there won't be that many places in core where we would do that and I think custom suspends are more likely to be the wait-for-async than this. But no strong opinion on this.
The idea here is to skip 4xx-response validation only for the customer profile bundle, based on the assumption that this bundle is owned by commerce and it being extremely unlikely that these profiles are pubic content, have aliases and stuff.
Since there can only be one bundle class per bundle, the alter hook checks if there already is one and doesn't replace it. And if a later alter hook replaces it again, that should still win. So this should be safe for BC and existing sites in case they already use a bundle class.
Your call if these are assumptions that work for you as a default, we believe that the result is valuable and results in considerably fewer 4xx-response invalidations on sites with lot ot checkout activity (and less frequent content changes on nodes/products/...). It is also possible to add this in custom code, we already did that and just wanted to contribute this back.
This only reduces it from 76 to 64 calls to usleep() and even with a 10 instead of 2 it's still 45 calls. I think a large chunk of rendering happens in a single render context, so we essentially suspend the same fiber many times right now if it's for the main content.
Since the scenario with a long-running query doesn't really exist yet, should we remove this for now?
Alternatively, it would be nice if we'd knew in the suspend-cases whether there is just a single fiber or multiple. I have no idea how close the event loop issue is and if it will allow to check that. Should we add our own basic global flag for that, something like FiberHelper::allowBulkSuspend(TRUE/FALSE)?
Or, fibers have a communication channel. Could we pass around some flags, like FiberContext::WAIT or FiberContext::RESUME to guide something like Renderer and whether or not it should immediately resume or wait a bit?
Two new merge requests created, this time I actually tested it, want to double check so I don't do something stupid twice?
Yeah, you're right, not sure what I was thinking.
See comments. both of those test code in those modules, not in text module. the job isn't done by moving the tests, we need to deal with the actual logic of the summary token and the ->summary property in editor module.
pathauto doesn't manage aliases. It just manages alias patterns and generates/updates them. pathauto does actually provide a migration for patterns, so I don't know why that wouldn't show up, but worst case scenario is that you have to reconfigure your alias patterns.
Adding some thoughts, I think we should keep the new tests in the module that they're testing the code for.
Updated the BC stuff. Thanks for starting the CR, I worked a bit on that as well.
One thing that's open is the remaining methods that still use the "listener" terminology. I had a look at this, however, the thing is that the ImplementationList currently also uses listeners (as in a hook listener, not an event listener), so might be fine. I did rename the alterEventListener property to alterHookListeners. It's still a bit inconsistent, also the docblocks. I did change one "functions" to "listeners".
There is also a @todo about sorting in ModuleHandler::add() but that's already deprecated and rarely used, maybe we can just remove the todo, nothing seems to be testing that.
> I measure rate by comparing hits vs misses, I observe the graph in NewRelic Redis monitor.
I don't know how exactly new relic works, but on all redis commands, this almost certainly meaningless if it measures whether or not a command returns a value.
If you can filter it to just hgetall requests, then that should give you a more useful statistic. But that will _not_ account for cache tag based invalidations, so it will be a hit on redis but then we'll see due to cache tags that it's actually no longer valid. So the number would be too god.
What you want to look for really is the ratio of hgetall to hsetall commands. That's essentially your cache hit ratio, because any actual miss will be immediately followed by a hsetall.
how exactly are you measuring hit rate? If that for example also measures lookups of cache tag invalidations as a miss then that will be quite misleading. It's very common that they don't exist if for example the node for node:1 hasn't been saved since redis has been started. That's not a "miss" as far the cache is concerned, quite the opposite.
I typically look at read/write data as a guidance for hit rate.
Using volatile over allkeys is recommended, because allkeys will also delete cache tag invalidations, which might result in unnecessary tag checksum fails.
That the page fails high redis maxmemory is known and there area some existing issues. There's also an issue to provide a similar report through drush, which would at least have fewer issues with time based limits. The easiest fix would probably be to add hard limit to how many items we check, similar to how we already do that for cache tags. If we reach that, display a warning and just completely abort the loop. Could also be memory based, similar to how drush has a check to restart if 80% memory is reached. we could check that every nth loop and abort the whole thing.
I pushed a change to that issue to deduplicate the two lists.
I'd vote to just drop those includes in D13 as we remove legacy hooks. Deprecating it will just confuse people because we don't really know if the files have explicitly been loaded or not. I think barely anyone explicitly relies on it being loaded. Webform has tons and tons of those legacy hooks in .inc files, but they're all loaded explicitly in the .module file.
What we could do is avoid pushing include files into both includes and group includes and I think that even fits into 🐛 HookCollectorPass registers event listeners but they do not follow the required calling convention Active because we introduce the new global includes list there and even have an explicit test asserting it's in both places. With that, the list will be a lot smaller.
I did a manual diff in my project to see what's left, it's lots of entries for hook_requirements() in .install files, various legacy drush.inc files, some dynamically loaded .inc files such as field_group_form_entity_form_display_edit_form_alter(). I also did find some actual non-converted hooks in webform.editor.inc for example (webform_filter_format_access), the uppercase implements docblock block that I mentioned before. But as mentioned, they are explicitly loaded.
I think the biggest risk is attempting to parse and run broken code, as modules might have old inc files with dead code that for example was never ported properly to D8+. But we're already doing that, apparently didn't run into any major issues so far and deprecating automatic including won't stop that.
Added comments. This is the canonical documentation for this module and linked from the project page, there is no documentation guide and no plans to add that.
30-40% does sound low. Did you check the report page the module provides? It might struggle to deal with 3GB of data though.
Make sure it's been running for a while, common things to look out for are frequent cache tag invalidations that are used a lot (e.g. having views on all pages with node:list cache tag). There are some issues to make that information more acessible. It's fine to have frequent invalidations, it's a problem when many caches depend on that. Or render cache items with a large amount of variations, such as blocks that differ for every page and user.
Feel free to reach out through my company MD Systems if you're interested in a more in-depth analysis, we do offer that.
It still is relevant, but it's also still postponed on the IEF issue, it only works when combined with that and that's a hard blocker.
There was already an MR, I rebased it, conflicted only on coding style fixes.
Yes, I meant converting webform_update_dependencies() to WebformInstallUpdateHooks.php is wrong and it's correct to revert this.
Not a maintainer, I'll leave the decision on what to include in this issue with the maintainers, just think it *might* be easier to get it committed without that. I don't know if tests against D11 ever worked, but the select2 change for example is easily something that could result in discussions, select2 has a new major version that is D11 compatible, so might want to allow that instead, but that might require other changes...
A second comparison, with a dynamic page cache hit so we have a few hook invocations and load the data still gives me considerable memory and network savings:
Peak Memory-3.38 MB (-16%)
21 MB → 17.6 MB
Network-629 kB (-43%)
1.45 MB → 0.823 MB
cpu/time is near-identical, with 0.2/2% difference, which is well below what I'd expect the variation between requests to look like. So the memory savings should not come with any time regressions.
In that scenario, getHookImplementationList() is called 23 times, the inclusive costs are uses 1.7% of time (716 µs) and ~5% of peak memory (762 kB). 37% of that time is loading the hook data cache. this was with the database cache. no other calls are visible, so I think there are no hook implementations in this scenario. Which I think shows that there is some potential in more efficient caching, but it's fairly minor.
When I disable all render caches, then getHookImplementations() is still around 1.3% (6.54 ms) and 9% of memory (3.82 MB), but it's also worth pointing out that 50% of the time is ImplementationList::__construct(), which means all those asserts, so on prod configuration, it should be half of that. And 30% of the time is getCallableFromDefinition, so instantiating the hook services. 2000 calls to the the method in total. And still 3MB less memory usage in this case.
Long story short, considerable memory savings without any measurable cost in response times for non-page cache hits.
Here is some data from our distribution:
container size 11.x HEAD: 1317261
container size with this MR: 687021
that means the container size is 52% of HEAD, so reduced by half.
Keyvalue size:
select name, length(value) from key_value where collection = 'hook_data';
+----------------------------+---------------+
| name | length(value) |
+----------------------------+---------------+
| group_includes | 22919 |
| hook_list | 99885 |
| includes | 11199 |
| packed_order_operations | 455 |
| preprocess_for_suggestions | 572 |
+----------------------------+---------------+
So obviously a lot more includes and stuff. And when all those things are converted to hook classes, it will be a bit bigger again, but still, that's a really big reduction.
Peak Memory-1.88 MB (-42%)
4.46 MB → 2.57 MB
Network-627 kB (-45%)
1.4 MB → 777 kB
Time-1.21 ms (-18%)
6.66 ms → 5.45 ms
Note: I uninstalled auto_unban for this comparison, which injects the config factory (which uses the event dispatcher) into the BanIpManager which is used by the BanMiddleware. In other words, 🐛 Regression: All stack middlewares are constructed at the same time even for cached pages Active isn't as effective yet for us as it should be. With that, the difference in network was the same (makes sense), but memory was a reduction of 64% and time savings were 28% because creating the event dispatcher also injected all the registered hook stuff and then that is a lot faster of course.
Should we split out the actual fix from the gitlab CI changes as this is definitely wrong and essentially just a revert to how it was?
@nicxvan: it seems sensible to assume that hooks can't start with a number. Should we add another explicit check for that for legacy hook discovery?
Created a new change record to group them together. Didn't find time yet to update the existing ones and cross-reference them as I had to leave.
The reason I change the order is that we don't want to call both the old and new requirements hook of the same module. for runtime and update, that's now taken care of automatically because it's no longer registered, but for install, it falls back to the legacy function exists stuff, as the module isn't installed yet.
In most cases, calling both might just be a slight overhead in case it does something slow (it shouldn't, but who knows), but there might also some extra weird edge cases when merging the results together.
I guess one obvious reason that \Drupal\Tests\navigation\FunctionalJavascript\PerformanceTest doesn't see much of an impact is that it doesn't do anything in this scenario. We just look at the frontpage without any nodes. Top bar really only does anything at all when viewing an entity, where it builds for example the local tasks.
Might want to do some manual tests or extend the automated tests, but it seems more useful to switch umami (and standard) asap to navigation and then we can drop this custom test anyway.
This postponed on one last issue: 📌 Remove BlockContentController::Add Needs work .
The deprecation for template_preprocess is in place and is correctly picking up some bits we missed. There's one in field_ui that looks converted because it's already a one-liner calling the service, but it's a redirection to another preprocess, this can now actually directly use that.
And one in views was missed too, that seems completely untested as the old function had been deprecated.
The module did. It's a post update, it automatically runs. If you don't see the effect of that post update, then you likely undid its config changes. Always export config after running updates, do not re-import config afterwards.
I haven't look at the test, many other tests set up modules in vfs instead of changing real files, would that be an option here?
Some final comments from me, I agree this is ready, but I'd prefer if someone else RTBC'd it, while @longwave made considerable changes, much of the underlying idea and approach was done by me.
Had a tricky test fail on workspaces update tests, The module install it does was running before the router table was updated and then that failed. Not exactly sure why this is triggered now.
You need a user that is allowed to access that entity. There's an admin permission defined on that test entity.
> OTOH it seems a bit strange to me that devel_entity_type_alter gets called while devel is not installed yet. Is that a concern?
As far as HookCollectorPass is concerned, devel *is* installed at that point, because it exists in the modules list in the kernel. I agree it could be a problem if a hook is invoked that relied on something from the module, but invoking hooks while rebuilding the kernel is an extremely bad idea anyway. It's going through multiple layers, it's also relevant that you have admin language negotiation enabled (\Drupal\user\Plugin\LanguageNegotiation\LanguageNegotiationUserAdmin), which is the thing that's doing a permission check, which in turn is what is causing the role config entities to be loaded, which is invoking those hooks.
That reminds me, the information when a BC layer is removed does currently not exist in a structured way in change records, that would be way more useful than the branch. Does someone know if an issue exists to add that? Especially now when not everything is being removed for the next major.
I think removing old template_preprocess functions in D12 is one thing, removing support for it might be pretty disruptive. On the other side, BC is built-in and just works (Drupal 11.2+ only looks for a template_ function if no explicit initial preprocess is specified), so it's pretty easy to do that, I recently updated token module accordingly. You don't need a LegacyHook attribute or anything, just leave the old function there and either call the new or just keep it as-is, depending on how much code there is. And it would also make it easier to also deprecate the file and includes keys in hook_theme for D12 (which aren't deprecated yet), because template_preprocess functions is really the only thing they are still used for. We an add an explicit deprecation in either the meta issue or a follow-up of it and finalize that there.
Updating metadata, this in image.module task not a feature.
An is callable is an option to avoid fatal errors, silently ignoring things is always a bit tricky if something unexpected happens, e.g. discovery of a callback that can't be loaded.
I think what should solve this problem in this case is moving the load() calls to the moduleHandler above updateKernel() in \Drupal\Core\Extension\ModuleInstaller::doInstall. I think that's consistent with how we register class namespaces as well, which also happens before the container is rebuilt.
@herved: I'm pretty sure that's a new and different issue than what this is about. Can you create a new issue? It would be helpful to know if this happens in 11.1 or only 11.2. It very likely certainly doesn't happen on 11.0.
And no, this doesn't have anything to do with config syncing, this isn't about changing the container. This happens in the middle of installing the devel module, when the container is built, it finds the hooks, but the module file is only loaded afterwards. And yes, invoking hooks as part of a container rebuild is more than just a little bit scary but I don't think that's new.
It looks like there were bad rerolls with the media Test. It's doing things twice now, the old edit keys can just be removed, the new helper takes care of that. See for example the patch in #75.
The node title is the entity label. You have a label in your test. The title for nodes is required, you can't save a node without one, that's why a test entity type is needed for this.
There doesn't seem to be existing entity in entity_test module that works for this, so adding a new test type is the correct thing to do here.
Yes, that file, and every line there is documented and explained. That's the recommended starting point. No change is necessary for a local setup with redis on default port. Several lines in your snippet are unnecessary/the default.
You're welcome to extend the readme files that come with this module, they are used to generate the docs.
The default/example configuration is already documented and exists as an example, I don't think that needs to be duplicated, just linked.
Thanks, I think this is ready now. This will conflict with 📌 Entity lazy multiple front loading Active and we'll need to rebase one of the issues once the first lands as they both change the same performance assertions.
Updated tests, made the failing test a deprecation test that we can then just remove as we have dedicatated tests for each new hook. I also removed the deprecation message from drupal_check_module(), HookCollectorPass will trigger that, whether it's install, update or requirements. The only change there is only calling it as a fallback. I think edge cases like having an install requirements class and not returning something is not really an issue, in that case the old implementation probably also won't return something.
Unsure about the change record situation. We have 3 separate, not-connected change records about the new hooks. We can a fourth that hook_requirements() is now really really deprecated and reference all with each other. But I think it would be easier for people if there were just one.
I still think we should have done this as part of introducing the new hooks (at least with the last of the replacements and conversions landing because the situation in 11.2 is confusing. We could backport this to 11.2 without the deprecation message.
berdir → changed the visibility of the branch 3490846-deprecate-hookrequirements to hidden.
> Are you able to get the size of the separate hook cache entry?
MariaDB [db]> select name, length(value) from key_value where collection = 'hook_data';
+----------------------------+---------------+
| name | length(value) |
+----------------------------+---------------+
| group_includes | 6 |
| hook_list | 47224 |
| includes | 6 |
| preprocess_for_suggestions | 169 |
+----------------------------+---------------+
So the key value entries take up just 20% of the size the container had, because I'm storing just the raw simplified structure of a single callback => module array, while the container contained multiple objects per entry. I can also shave off a little bit more, because we can remove all the skip_procedural_hook_scan flags, they're not needed at runtime.
preprocess_for_suggestions is tiny, core doesn't have a lot of those, on smaller projects there can be quite a bit more. I'll rerun this on our distribution when this is closer to working.
> I assume this diff includes it but it would be good to know what it looks like.
No, the diff is specifically without it, that's kind of the point. This is on a page cache hit, which doesn't invoke any hooks. The current implementation loads all hook data at once, but on demand on the first hook invocation. Right now not even a cache, but that's one of the things I want to add next.
> I'm wondering whether it would be feasible to key the separate hook cache by whether there's a session or not (or similar) since we can generally assume users with no session won't be discovering lots of form alter hooks, hook_requirements() and similar.
my plan is to make the hook data lookup internal and easier to optimize and refactor but keep it simple in the first step, just a single cache item, which is already loaded later and much smaller than HEAD. It's also not unlike the old hook cache, we also just had one that we gradually built up. this has the adventage that have the complete data from the start, no race conditions/multiple writes. At this point, I'm not certain that it's even worth optimizing further. Cache variations mean more complex invalidation/checks (cache tags), more likely to have multiple conflicts between multiple requests (e.g. a cache collector or so). And I suspect it will be hard to find good indicators for what kind of hook data is needed. Anonymous requests can also hit a large amount of hooks if there was a render cache tag invalidation for example.
I've been working quite a bit on my approach. I want to stress that it's unfinished, messy but my focus was on figuring out what we can gain in regards to container size, specifically on page cache. In regards to the implementation, there are plenty of details to figure out, caching, what's the responsibility of ModuleHandler vs ImplementationList, if we even really still need that and so on. My latest refactoring also broke the unit and kernel tests again that I had mostly fixed before, will need to update some identifiers and stuff, so not yet opening a MR.
I've also moved preprocess_for_suggestions now, this is information that is only needed on theme registry builds and definitely shouldn't be in the container.
On 11.x HEAD, the length of the container cache entry is 740937. With my MR, it's 488678. that's a reduction of 35%.
Testing with blackfire 11.x against my branch on a page cache hit gives me the following reductions:
Time: 15% (5.69 ms → 4.82 ms)
IO: 18% (2.05 ms → 1.69 ms)
CPU: 14% (3.64 ms → 3.13 ms)
peak memory: 23% (2.68 MB → 2.07 MB)
network transfer: 29% (0.914 MB → 652 kB)
I wouldn't put too much trust in the time metrics, those vary quite a bit, but the memory/network metrics should be pretty stable. And the numbers track fairly well with the measured container size.
Created #3549088: Use fibers in BlockRepository::getVisibleBlocksPerRegion() → as part of the meta, also grouping this under that meta.
Fully agreed with #67, to elaborate on "but also it doesn't do anything when render caches are warm because path aliases don't need to be resolved at all". It's not just that it doesn't do anything, but also that if there is one to look up, it loaded a possibly large amount of aliases that aren't actually needed because those parts are still render-cached.
And, due to 🐛 Feature or Bug? - The path-alias preloading is not used if the "Enforce clean and canonical URLs" option is enabled Active , the whole feature didn't do work at all on a vast amount of sites for a probably very long time.
Just some minor remarks on the MR, like the deprecated version, then this is RTBC for me.
I thought we don't ignore own deprecations but I see there are quite a few in the list right now.
Ok, I think we're good here (again). Pretty excited for what this, the path alias and some other issues will allow us to do.
Speaking of that, @catch, I'm wondering if we should open an issue to "fiberize" \Drupal\block\BlockRepository::getVisibleBlocksPerRegion() to handle the b lock_content load entities. Maybe only some where access checks are non-trivial, I don't know what the overhead of fibers is. Some sites have a large amount of blocks. On umami, that would allow us to group those 3 block_content entities.
I also had the idea that we could possibly generalize this into an attribute that you can provide a version for, because we'll deprecate more hooks where you'll want to keep the old one. Your build-time deprecation issue will run into that a problem because we won't be able to have conditional runtime logic like I'm doing with install here.
Something like #[HookSkipAfterCoreVersion('11.1')]. It allows you to skip and acknowledge that you just provide that hook for compatibility with earlier versions and want it skipped now.
I've been thinking about this and I think we should just add another dedicated legacy hook attribute. That solves the runtime issue, becaause those hooks just don't exist anymore.
That just works for update/runtime because those are regular hooks. And install requirements are far easier because they area done one-by-one and have no alter, so we can just put the old hook in an else.
Needs review to get feedback on the concept, this will fail tests obviously.
I thought a bit about how this would work and it's not trivial I think, IMHO, changing what view() returns is problematic, there are going to be so many things that alter and customize that. We technically don't have BC for that, but it's a pretty central piece of what we do and something that's often customized.
For example, something like comment_preview() would be an exception, because you can't add that to a lazy builder. Things like node/comment views Rss row plugins would break too.
We'd also need the ability to opt out for entity types, block_content is already in a lazy builder.
The complexity here is because we can't rely on LegacyHook due to 11.1 and we don't want to invoke the legacy and new hook for the same module, not the actual deprecation. If we'd deprecate the old hook then modules wouldn't have a way to avoid that deprecation message.
Yes, exactly. It's a small change but at least for me I think easier to grasp what's happening. The $entities ternary could also be a regular if () now as the else is just reassigning $ids to $ids, but that's up to personal preference.
I have no idea how I came up with #14, I meant comment #50. edited my comment. You don't have to delete your comment, I can live with that mistake being out in the open ;)
I agree with #14. I started testing my ideas a bit in the other issue and I understand a bit better how these currently work, in that they aren't really services but rely on having DI and get the resolved hook services injected. This would need to change with my approach so we might need to revert and change considerable aspects of this again.
> We've got media items that are translatable and the edit form shows the translation + authored by fields. If you change the standard authored by, it's value is ignored and the media item is updated with the translation author field instead.
That's not a different issue, that is _exactly_ what led us to create this issue 7 years ago :)
> Is there a summary of what's left to do on this issue?
Nothing really. It was RTBC 2 years ago and then it ended up in reroll limbo.
There is a slightly awkward situation exposed by the current block_content status where this might expose fields that are currently hidden by other means. This would go away for block_content if the status would finally become visible there, but that issue too was stuck in limbo. But IMHO, that possible issue is far less severe than the current highly confusing state on media translate forms.
reviewed a bit. since this also does non form alter stuff, maybe retitle to remove and deprecate functions from content_translation.admin.inc or so?
I still think switching to $ids earlier would be easier to understand, but that's a minor issue, this is definitely better now.
The unit test changes are consistent with the other lines in that provider. Very weird, but consistent. Really not fond of those unit tests, but that's a different topic.
Lets do this.
I started with a rebase of one of the MR's. The new tested I added to HookCollectorPassTest is going to fail. Will reply to #51 and plan to work on my idea soon.
Now with 🐛 Regression: All stack middlewares are constructed at the same time even for cached pages Active , the relative impact of loading the container for a page cache hit is even bigger.
Lets do this, then. We had an interesting case recently where we moved node logic out to hooks due to it being optional and conditional on the search module, but that is not the case here.
Renamed the class, added some more usages, created a CR.
Writing out debug statements into files helps me understand what's going on, so I did that again here with this diff:
diff --git a/core/lib/Drupal/Core/Entity/EntityStorageBase.php b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
index 05aeb6587bb..bc5c15c464c 100644
--- a/core/lib/Drupal/Core/Entity/EntityStorageBase.php
+++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
@@ -297,7 +297,11 @@ public function loadMultiple(?array $ids = NULL) {
// of entities to load, so that another call can load everything at
// once.
$this->entityIdsToLoad = array_merge($this->entityIdsToLoad, $ids);
+ file_put_contents('local/load/' . $_SERVER['REQUEST_TIME'] . '.txt', 'SUSPEND: ' . $this->entityTypeId . ':' . implode(',', $ids) . "\n", FILE_APPEND);
$fiber->suspend();
+ if ($this->entityIdsToLoad && $this->entityIdsToLoad != $ids) {
+ file_put_contents('local/load/' . $_SERVER['REQUEST_TIME'] . '.txt', 'BULK LOAD: ' . $this->entityTypeId . ':' . implode(',', $this->entityIdsToLoad) . "\n", FILE_APPEND);
+ }
// Another call to this method may have reset the entityIdsToLoad
// property after loading entities. Combine it with the passed in IDs
// again in case this has happened.
@@ -317,6 +321,9 @@ public function loadMultiple(?array $ids = NULL) {
// entity static cache itself).
$this->entityIdsToLoad = [];
}
+ else {
+ file_put_contents('local/load/' . $_SERVER['REQUEST_TIME'] . '.txt', 'SKIP: ' . $this->entityTypeId . ':' . implode(',', $ids) . "\n", FILE_APPEND);
+ }
}
// Try to gather any remaining entities from a 'preload' method. This method
so, I write out a line into a file in 3 cases. when suspending, with the requested ids. on resume, if we successful in aggregating multiple requests together and skip, when loading something and there's no active fiber.
On umami frontpage with disabled render cache, this results in
SKIP: user_role:authenticated
SUSPEND: node:10,9,8,7
SUSPEND: media:21
SUSPEND: user:6
SUSPEND: user:1
SUSPEND: file:41
SUSPEND: media:9
SUSPEND: file:17
SUSPEND: media:8
SUSPEND: file:15
SUSPEND: media:7
SUSPEND: file:13
SKIP: block_content:3
SKIP: block_content:1
SKIP: block_content:2
SUSPEND: shortcut:1,2
SUSPEND: node:3
SUSPEND: node:18
SUSPEND: taxonomy_term:1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16
SUSPEND: node:19
BULK LOAD: node:3,18,19
SUSPEND: media:18
SUSPEND: media:17
SUSPEND: media:19
BULK LOAD: media:18,17,19
SUSPEND: file:35
SUSPEND: file:33
SUSPEND: file:37
BULK LOAD: file:35,33,37
What surprised me a bit is that basically everything is in a fiber, that's because of the fiber in \Drupal\Core\Render\Renderer::executeInRenderContext(). the only exception is the 3 block_content entities being loaded as part of block access checking.
First are the nodes from the main page view and then they are being rendered, and this isn't working yet in parallel, so we'll need the views fiber issue or maybe 📌 Use #lazy_builder / placeholdering for entity rendering Active for this.
Then it can load 3 nodes in bulk, then 3 medias and then 3 files. two nodes are from link fields and their access check on content blocks, and the third is from one of the views. I was confused why the second views block didn't show up, but that's because those two nodes are already loaded from the main view, those articles show up twice. medias are then a mix of one from the article and from the blocks.
=> Overall, shows that this works nicely. It aggregates all the things from all the blocks until it went through all of them, and then repeats. Even across different entity types as visible by the media case. Neat.
Added some comments.
I tested with the views issue, didn't see a difference yet, @catch mentioned that in #75 as well and that makes sense. This is by far the easier change, it works, and we then can enable it to work on more cases and test the impact then.
Rebased, only had two conflicts on test classes, some use and docblocks in core/modules/views/tests/src/Kernel/ViewsPreprocessTest.php and core/modules/views/tests/src/Kernel/Handler/ArgumentSummaryTest.php. Should be fine to set back to RTBC assuming that test are green.
Similar concern as in the views issue, special cases such as paragraphs that don't use render caching. For a lazy builder, we could probably combine it with render-cache enabled check, although that couldn't account for cases that remove the cache keys later, such as \Drupal\node\Controller\NodePreviewController::view. On the other hand, paragraphs could possibly benefit a lot from this, as it's common to render a lot of them, with many references to medias, other entities and so on.
I'm still getting familiar with fibers. I did a quick test to just "fiber-ize" EntityViewBuilder::build(), essentially the call there to buildMultiple(). It kind of works, but it didn't quite have the effect that I expected. I think I'm starting to better understand that for fibers to be effective, you need to have multiple of them and loop over them. I thought we can possibly build up some fibers from different entities being rendered as we go, but I think it's not quite so easy.
Somewhat related: I think we should consider to deprecate buildMultiple() and the ability to build multiple entities at once. It's broken ( 🐛 getViewBuilder('node')->viewMultiple() bypasses render cache Needs work ), barely used (comments pretty much being the only case in core AFAIK and they rely on it being broken). Instead, we'd shift towards fibers and stuff to build more entities in parallel.
re the array_replace() stuff. I don't think #75 is correct and I think it the current test only works because we only do a single load in each fiber. We should imho extend the test with an loadMultiple() call and make sure we only return requested entities.
If I set a breakpoint on umami with the breakpoint condition in #72 then I see this after I run through to the array_replace():
// The originally requested ids.
$flipped_ids = [3 => 0];
// The enriched $ids based on $this->entityIdsToLoad
$ids = [3, 18, 19]
// And $entities contains 3 items for 3, 18, 19 after the replace.
$entities = [3 => ..., 18 => ..., 19 => ...]
So, loadMultiple() definitely returns 3 entities when only one was requested. This only happens for the fiber that is resumed first, because for the others, $this->entityIdsToLoad has already been reset, so the merge there ends up with just the requested ids. That means to reproduce this, we'd need to have the loadMultiple() call first in the explicit test.
This doesn't fail on umami, because EntityStorageBase::load() explicitly returns the entity for the requested id and \Drupal\Core\Entity\EntityRepository::getCanonical() returns the first items, which is also guaranteed to be the requested one for a single load.
Looks good to me, but needs a rebase, as 📌 Explore inverting checkForProceduralOnlyHooks to prevent them from being added as listeners. Active just landed.
Just one kernel test failed, that didn't have a the required dependency enabled, fixed that.
On D12 vs D13, I don't have a strong opinion. There are a handful of contrib projects using some of these functions. I usually default to D13 at this point unless I have a reason to push for D12, it costs us very little to keep those handful of functions in the module file, we have tons like that and the performance and technical dept cost doesn't really hurt us I think.
Accepted that, makes sense. I had just 'requirements' instead of hook_requirements as I was referring to the list of strings in the other method, but either way is fine for me.
Comment #4 has some numbers, although that's outdated as requirements are now again in the list.
> This is getting super tricky to deprecate extension.list.theme_engine while still actually needing to use it for various tests, maybe have to revert this and leave it as an unused service that we can just quietly remove in Drupal 13.
That's what I expected and didn't even try. My vague idea was that we'd just leave the extension list in place, it won't find anything anymore in D13 as engines are no longer supported and we could either deprecate it for D14 or remove it immediately.
🌱 [policy] Standardize how we implement in-memory caches Needs work has a similar chicken/egg situation with the cache.static service. We need to access it as long as it still exists for invalidation. @catch suggested to then ignore the deprecation message, but I don't really see a difference to not have a deprecation in code. We could also just have a change record without forced deprecation? I'd imagine that this service is very, very rarely used, there's no reason to interact with it directly.
Wasn't quite sure about the other functions. The private/not private indicator feels completely arbitrary, some non-private functions are only used by other private functions. All of those functions have a bunch of usages in contrib. Including the template_preprocess ones. Wasn't sure if I should expand the CR to cover those as well if we have one anyway and then rename it to say something like Functions in responsive_images.module deprecated
Also open to suggestions on the class name for the new service. Builder was the best I could think of as it's all related to building responsive image attributes.