#Performance

Put an MR up.

Feedback from the original MR appears to be addressed in the new.

One thing I just realized by reading this IS, is that 🐛 Node update hook is triggered upon group content create RTBC did not add list cache tags to its solution.

So if you had a list of 5 nodes you had access to due to how they were grouped and you lose that access, we are fine because the list was tagged with both the individual nodes' cache tags and node_list. However, if you gain access because a node got added to one of your groups, the list won't show it because we no longer invalidate list cache tags.

Now, to the issue at hand, hook_entity_access() does not apply to lists, so the facepalm I just had should not get worse if we do as the IS suggests. And as you mentioned, adding or removing an entity from a group will either save the entity (v2/v3) or invalidate its tags (v4). So a main menu block should always get updated when one of the nodes that are actually in the menu have their group status changed.

So I think you're right and we can completely remove that piece of code.

smustgrave → closed merge request !1529

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

Facing this issue with Drupal 10.4.7

📌 Views entity rendering should use a lazy builder/placeholder Active is one such case where we can assume it's likely that multiple links/urls are involved.

Another one which doesn't have an issue yet is local tasks/actions.

Those would definitely pick a lot more paths up and be less dependent on what's going on in the rest of the page.

> So if three blocks have one link each, three links will be loaded in one round trip. But if one block has three links, it will require three round trips - because the first link has to be loaded before the second.

For routes, we have \Drupal\Core\Routing\PreloadableRouteProviderInterface::preLoadRoutes(), which is used by the global preloader that I'm trying to change, but we also call it in \Drupal\Core\Routing\RouteProvider::getRoutesByNames(), which used for example by \Drupal\Core\Menu\MenuLinkTree::load to preload all routes for a given menu.

Maybe as a follow-up, we can think of something similar for specific cases where we know that we have multiple links/url objects, we can explicitly request those to be preloaded? We need to make some assumptions and path aliases did move into a technically optional module now but maybe we can think of something, could go through the UrlGenerator and an event?

So just like my route preloader issue, we still have the concept of preloading, but we move from global preloading to per-block/element preloading, which fits better with partial/render caching.

As requested giving this one a review.

I'm new to Threads but the loop makes sense to me from the examples given. If I have understood it correctly then the answer to my question on the MR should be 'none'. If that's the case then I'd say the logic here is solid (and we can remove a couple of unset commands).

My only other concern is I don't think we're testing the non-thread use-case? I know they were introduced in 8.1 and 8.0 is out of support, but what if someone is running a NTS binary or something?

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

> Cache tag isn't great since Drupal de-dupes multiple invalidations.

Yes, but only if there were no new caches set since the last invalidation, that's not an issue.

However, cache tags still need to be actually invalidated, and there are _many_ dynamic permissions based on bundles and other config entities, all those would suddenly need to add cache tag invalidations.

This is going to require a lot of work and it might be easier to deal with specific issues on a higher level. For example, the recipe action could support a list of permissions instead of saving the role config entity 30 times, that's always going to be slower.

This has been rebased since the bot marked it needs work.

I just ran into this again. I had a Recipe that applied 30 permissions. It took 2 seconds because the role validation causing `\Drupal\user\PermissionHandler::buildPermissionsYaml` to run at each granted permission. I'm guessing \Drupal\Core\Extension\ModuleInstaller::doInstall would need to reset cache on each install.

Cache tag isn't great since Drupal de-dupes multiple invalidations. But then the module handler is aware of the cache ID. Unless PermissionHandler has a specific "reset" method added.

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

On the specific site I'm testing with, this works if I change 'full' to 'default' when attempting to load the display, but otherwise it still falls back to ::getDisplay(). This probably isn't viable because it's specifically looking for 'full' in ::getDisplay(). Including an xhprof screenshot showing a definite improvement when it 'works' though.

Hmm not as convinced on this after some more changes to get tests running. Got a passing PR up anyway. NavigationContentTopTest had a ~12% increase in speed with the new PR averaging out over 3 runs locally.

Had a bit of a play with this, just trying out an alternate approach. I don't think we need to run all the entity queries at all for getting the cache data. We already know the display we want to cache just by the entity & entity type ID

The only thing we might lose here by loading the entity directly is that the alter hook might not run. If thats an issue we can also run the alter hook here potentially 🤔

I think this is OK in a minor - if we add a change record and release note for it. The release note can point to the contrib issue.

Tried https://www.drupal.org/project/metatag_async_widget → , resolved the issue for me

It reduces edit form load time from 25+ sec to 4 sec, but still, when we try to open the metadata, it takes time

I checked and found that there are multiple metatag modules enabled for different groups and schemas

Disabling that reduces the load time significantly

Just to expand a bit:

The current label (title), "Show the same menu markup on every page", does not give enough information, so the user has to read the description (help text) to figure out what it means.

I think that

Add the active-trail class

has exactly the same problem, or is possibly slightly worse - e.g. only a tiny, tiny fraction of experienced Drupal users will know what the 'active trail' class is, so everyone will need to read the description anyway. But by making the active trail concept the main thing, it becomes more important to understand what it is, vs. deciding that 'yes, I do want my main menu to look the same on every page'.

Yet another option would be to add the configuration option without any UI, the problem with that is discoverability of what can be a very significant performance improvement.

Random thought would be to do some sort of check in the status report, though the detection would likely be limited to globally placed menu blocks, and maaaaybe menu blocks in LB defaults. But then being able to set the config property would require some pretty technical knowledge, whether setting via drush or the config sync UI or whatever.

Add the active-trail class

I tried to avoid this when we originally started work on this issue, because the concept of the 'active trail' does not exist in the UI anywhere else in Drupal core and I'm not sure we should introduce it here.

By highlighting it as the checkbox label, it promotes the concept above where I think it should be for users (ideally not at all, but minimally introduced by this MR out of necessity). #33 / #34 show exactly how difficult it is to keep track of the difference between 'active' and 'active trail'.

Yet another option would be to add the configuration option without any UI, the problem with that is discoverability of what can be a very significant performance improvement.

I'm not sure what the best path forward is.

After making the switch in the UI from essentially "ignore active trail" to "show active trail", if we flip the config property to match, then we end in the position of needing to update existing block configurations and how to deal with them in layout builder. We could just update global block configs, leave LB blocks unchanged, and then in code treat the "show_active_trail" property being unset to be the same as TRUE.

Alternatively, we could leave the property as "ignore_active_trail", and then in the block config form submit, flip the form value* before saving to config. I don't know if that's done anywhere else though, and it seems counterintuitive and bad DX.

*I think there's also some consideration needed on how handle the form value because the checkbox can be disabled by states, though maybe the easiest thing would be not to disable the field and only set visible/invisible.

I am sure people are using custom code or contrib modules to add fields to webform submissions. If we can switch dynamic fields off by default and get a performance gain, that should be okay because people can turn dynamic fields back on as needed.

I took a look at converting EntityFieldManager::getFieldLabels() to use the field map and it doesn't help unfortunately.

The field map doesn't contain the field data itself, only which bundles have a field. Because this includes base fields/properties, every bundle is included in the field map, so you end up calling ::getFieldDefinitions() on all bundles anyway when building views data because it wants the field labels for base fields too.

If we knew which fields had overrides or not and which ones were base vs. configured in advance, we could pre-filter, but that information is from ::getFieldDefinitions(), so short of storing that in key/value too, it's just circular.

Opened an issue anyway since it might help in cases where you really only want the field labels for one field 📌 Use the field map in ::getFieldLabels() Active .

It's complicated by the menu allowing you to show which levels to use. So you can have a four level menu, only show the top level, but the three levels underneath all impact the active trail.

On the front page of Umami after a cache clear, I still get 15 calls to FieldHooks::entityBundleFieldInfo() - didn't check how many total bundles there are, I think this is from views data as you say.

On Umami this is about 330ms for me, which is not tens of seconds bad, but it's still 330ms that will block rendering of a lot of pages on a site (e.g. anything with a view on it).

For me webform + jsonapi is an extreme case and we should definitely do 📌 Change JSON:API routes from every bundle/field to per entity with arguments Active for that case, but we have a lot of code paths calling EntityFieldInfo::getFieldDefinitions() bundle by bundle and should either try to optimize that or convert cases to using the field map.

I see. That is different from what I worked on a few years ago. There, the navigation came from the book module, and it contained the full structure of the book. So all my JS had to do was find the current page in that tree and then climb up the tree, adding the active-trail class.

Ah, jsonapi, yeah, I didn't test with that. The call chain for me was views data.

Maybe 📌 Change JSON:API routes from every bundle/field to per entity with arguments Active would help then? I'm assuming those calls are coming from route building. If we have those generic routes, then the need to fetch all fields and bundles would just go away... not a quickfix like this obviously.

We add the 'active' class in JavaScript, which is based on 'is the link pointing to the actual page you're on', but we can't easily do that with the active trail class because it's based on a calculation of where the current page sits in the menu tree - the link can be added to the parent of the page even when the page doesn't actually show in the menu at all, the only way to add it via JavaScript would be to do an AJAX request, or calculate the active trail out of band and add it to Drupal settings or something then have javascript add it from there. It would also be JavaScript that's required to render pages to anonymous users whereas currently zero JavaScript is at all out of the box.

I can see it's coming \Drupal\Core\Entity\EntityFieldManager::getFieldLabels(). But that is only called if there is a field storage for a given entity type. so I assume you actually do have a configurable field there?

That code loops over all bundles just to figure out which bundles have a given field. We already have this information, in the field map. Maybe we can prevent this higher up in the chain?

This isn't how I'm reproducing it but it might be a similar pattern causing the problem.

Uploading an xhprof screenshot that shows the callers (to Drupal\Core\Entity\EntityFieldManager::getFieldDefinitions) , there are 965 bundles on the site (not all webforms, also paragraphs etc. but webforms are about 2/3rds), so each caller with 965 calls to it is looping over every bundle (unless it's an amazing coincidence).

The main culprits are:

Drupal\jsonapi\ResourceType\ResourceTypeRepository::getAllFieldNames() Drupal\jsonapi\ResourceType\ResourceTypeRepository::getFields()
Drupal\jsonapi\ResourceType\ResourceTypeRepository::calculateRelatableResourceTypes()
Drupal\search_api\Plugin\search_api\processor\ReverseEntityReferences::getEntityReferences()

It might be that we need to follow a similar approach to your suggestion above in all four places and add some docs discouraging people from looping over all bundles.

Confirmed that none of the webform bundles have a configurable field fwiw.