🇨🇭Switzerland @berdir

Yes, 3 fewer is exactly what I'd expect, 2 redirects for the search block and one for branding that we cache statically. I think trying to statically cache the cache hits as well is not worth the complexity. It's an exception that you really render the same cacheable render array multiple times on the page, like when doing that kind of twig customization as umami does.

I opened 🐛 Umami page template renders header regions multiple times Active for that, I also have some vague ideas on how to improve that in general, but that could be very challenging with BC (the problem is that render arrays aren't passed around by reference to twig commands, so we don't benefit from the fact that the renderer stores the built thing in #markup/#children).

The wording of some comments seems a bit unusual ("You can read up..", usually we just use a neutral "see xy" but there are a handful of existing "You can ..." in docs already) and slightly repetitive (the array keys are kind of explained twice for $redirectChainCache). But I'm not a native speaker and this is complex stuff, it doesn't hurt to be a bit verbose I think.

Implementation looks good now to me.

Tested, works fine.

kristen pol → credited berdir → .

I know. I'm not saying that's the problem. The problem is that redirectChainIsValid() incorrectly still assumes that we do store a full chain including hits in $chain and it's validating it under that assumption. As a result, it's silently failing those validations and _not_ using them in case of cache hits.

So either we make it less strict for those cases and not validate the last item if that's a redirect, or we stop pretending that we support that and _only_ store chains that end in a miss. This would have the same result with the current performance tests right now. Usually you'd expect that with the optimizations that we now have in place, once we have a cache hit, we no longer need the chain as we won't need to look it up again. Except the umami tests show that with the current twig approach (which is quite common), we do in fact render the same cacheaable element multiple times on a single page, so being able to at lest keep the redirects is useful in that scenario.

berdir → created an issue.

I had a closer look at what exactly those 24 cache gets for the NodePage cache are. The first 10 are rendered nodes and medias, that makes sense.

But then it gets interesting, I get 3x the umami_search block:

  [10]=>
  string(162) "entity_view:block:umami_search:[languages:language_interface]=en:[theme]=umami:[user.permissions]=77981d129fc903b842d6e469f96b1692c3ac47e0369f7b7876856518ae7131b3"
  [11]=>
  string(190) "entity_view:block:umami_search:[languages:language_interface]=en:[languages:language_url]=en:[theme]=umami:[user.permissions]=77981d129fc903b842d6e469f96b1692c3ac47e0369f7b7876856518ae7131b3"
  [12]=>
  string(162) "entity_view:block:umami_search:[languages:language_interface]=en:[theme]=umami:[user.permissions]=77981d129fc903b842d6e469f96b1692c3ac47e0369f7b7876856518ae7131b3"
  [13]=>
  string(190) "entity_view:block:umami_search:[languages:language_interface]=en:[languages:language_url]=en:[theme]=umami:[user.permissions]=77981d129fc903b842d6e469f96b1692c3ac47e0369f7b7876856518ae7131b3"
  [14]=>
  string(162) "entity_view:block:umami_search:[languages:language_interface]=en:[theme]=umami:[user.permissions]=77981d129fc903b842d6e469f96b1692c3ac47e0369f7b7876856518ae7131b3"
  [15]=>
  string(190) "entity_view:block:umami_search:[languages:language_interface]=en:[languages:language_url]=en:[theme]=umami:[user.permissions]=77981d129fc903b842d6e469f96b1692c3ac47e0369f7b7876856518ae7131b3"

And then 2x umami_branding:

  [16]=>
  string(164) "entity_view:block:umami_branding:[languages:language_interface]=en:[theme]=umami:[user.permissions]=77981d129fc903b842d6e469f96b1692c3ac47e0369f7b7876856518ae7131b3"
  [17]=>
  string(192) "entity_view:block:umami_branding:[languages:language_interface]=en:[languages:language_url]=en:[theme]=umami:[user.permissions]=77981d129fc903b842d6e469f96b1692c3ac47e0369f7b7876856518ae7131b3"
  [18]=>
  string(164) "entity_view:block:umami_branding:[languages:language_interface]=en:[theme]=umami:[user.permissions]=77981d129fc903b842d6e469f96b1692c3ac47e0369f7b7876856518ae7131b3"
  [19]=>
  string(192) "entity_view:block:umami_branding:[languages:language_interface]=en:[languages:language_url]=en:[theme]=umami:[user.permissions]=77981d129fc903b842d6e469f96b1692c3ac47e0369f7b7876856518ae7131b3"

The reason these multiple calls happen is because of core/profiles/demo_umami/themes/umami/templates/layout/page.html.twig, specifically this block:

  {% if page.pre_header|render|trim is not empty or
     page.header|render|trim is not empty %}
    <header class="layout-header" role="banner">
      <div class="container">
        {% if page.pre_header|render|trim is not empty %}
          {{ page.pre_header }}
        {% endif %}
        {% if page.header|render|trim is not empty %}
          {{ page.header }}
        {% endif %}
      </div>
    </header>
  {% endif %}

So pre_header is being rendered a total of 3 times. Not sure why the template bothers at all with that, it's a demo, it's not really meant to be customized. But it could store those things in variables. But that's a separate issue that I'll create.

However, I'm also not seeing the static cache kick in for those redirects, that would save another 2 lookups for umami_search at least for now. The reason for this is that redirectChainIsValid checks that the resulting cache id for the redirect chain is in $chain. But because we don't persist the cache hit in the $chain, this will never be TRUE for any lookups that were a HIT, it only works for misses.

I think that's in scope to be fixed here. Not exactly sure what's the best approach. Maybe we only need to validate entries that have a FALSE at the end of the chain? or specifically skip the last item if it's a a redirect?

local git rebase automatically identified the cache tags preload commit I included as redundant, so this should be fine, lets see.

Yeah I was also wondering this - if it's a 'normal' bug and the fix is self-contained (e.g. doesn't require adding some kind of declarative API) then it doesn't feel like a blocker.

I was wondering about that.

We can have a self-contained improvement to cover more cases, but it's probably not perfect. I've seen some discussions about what and if there should be a primary action on views pages for example, specifically admin pages such as admin/content and admin/people and whether the action there should be add user or edit view. Neither will be covered by this. But add user would be possible through the local action system that we already have.

berdir → changed the visibility of the branch 2232375-make-language-switcher to hidden.

Still postponed, but I've already rebased this on top of 📌 Try to preload cache tags in cache getMultiple Active to confirm my assumption that it makes sense and the answer I think is yes.

It's really cool to see how all these performance issues don't just add their improvements, but they actively combine to result in even nicer improvements.

This reduces the cache tag lookup increase for the dynamic page cache hit that this issue has on top of HEAD (3 to 8) to just 3 to 5. The reason it's multiple is that we have 3 groups of cache tag lookups. One is the menu cache tags being separate from the active trail cache lookup for the cache contexts, then we have a filter format cache tag from RenderCache::getMultiple() because that one apparently doesn't have a cache redirect and then all the other placeholdered blocks together. The reason it's a regression on HEAD is that previously, they would have been part of dynamic page cache, even though we explicitly asked for them to be placeholdered. This might sound like a regression, but it's actually good, because we want them to be separate from dynamic page cache, I think the current behavior also mean that autoplaceholdering is pretty busted because on cache hits on the inner elements, they still bubble up their expensive cacheability metadata to dynamic page cache.

And in multiple other cases with dynamic page cache misses, this saves 3-6 additional cache tag lookup queries in many test scenarios due to them now being multi-loaded.

Attempt at a new title

Created a MR for the interface backport.

> It was also concluded some type of access checking cannot be implemented as a query level access check, only as a single entity level access check which means the $entity->access() calls remains a necessity even if they break pagination or may lead to performance issues.

I don't agree with this. Core doesn't do it in hardcoded lists, views doesn't do it. Doing query access is expected to be sufficient. If that _were_ a necessity, then the fact that core doesn't do it would be a security issue.

As quickly discussed on slack, changing how the links are build is challenging because it's a pluggable system depending on your language negotiation settings. See \Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationUrl::getLanguageSwitchLinks, session/cookie based language adds its own query parameter that it then uses on the given request to put it back into a cookie. It's not impossible but tricky with BC and relying on this happening and so on.

FWIW, I'm reasonably certain that building this is faster than fetching from cache, based on renderering time debug output (had to hack it to show times also for non-cacheable elements), it's also the same right now in HEAD. But there might indeed be edge case such as it being the only thing it has to render with twig.

I also verified that this fixes OpenTelemetryFrontPagePerformanceTest.php with 🐛 [pp-3] Bubbling of elements' max-age to the page's headers and the page cache Postponed , which currently fails as this block prevents it from being a page cache hit.

It's also no change compared to HEAD. The block is currently being autoplaceholdered and not cached as well, just also in a way that that information up.

I also created 📌 Add CacheOptionalInterface to more blocks Needs review as a follow and want to add a change record for this.

berdir → created an issue.

the route name is just as much convention as the local task, it's probably more reliable than the local task as route names are typically generated while local tasks aren't yet.

Could go a step further and look for __entity_form in the route definition, but that's also an extra lookup.

Either way, doesn't really seem like a stable blocker to me? extending this to support more cases won't require API/structure changes or anything like that?

Issue summary updated

Updated, also shortened the test method description, it was already over 80 characters and that made it even longer.

✨ Use modal in add new field flow Active completely removed the exception. In manual testing, I don't even get far enough because there's an assertion in \Drupal\Core\Field\FieldTypePluginManager::getGroupedDefinitions(), but that was there before and it's not as helpful I think. Maybe instead of that assert(), we should throw the exception there?

Leaving at needs work, need to update the issue summary with the options and questions.

Rebased.

Rebased.

In https://git.drupalcode.org/project/drupal/-/merge_requests/10999#note_45..., yes.

FWIW, I specifically suggested to not introduce a helper service for the node indexing and keep them as protected functions. It's not an API that taxonomy module offers, so it makes more sense to me as protected methods. Yes, the logic is duplicated then on the functions, but I don't think there is a rule against that.

Entity browser and our project tests break completely with this, something here isn't correct.

Looking at the response, it has no cacheability metadata at all. $response->addCacheableDependency(CacheableMetadata::createFromRenderArray($preview)); isn't working, $preview not yet rendered at this point, the cacheability metadata is in $preview['view'] and not picked up by createFromRenderArray().

IMHO, this shouldn't rely on the rendering to set cacheability metadata on there IMHO. The controller implements and uses specific query parameters, such as view_name and view_display_id. It should IMHO always explicitly add those specific query parameters or just url.query_args to the response, otherwise there might be edge cases that might be missing them, for example a weird display that outputs something unexpected?

📌 Add JSON:API performance tests Active is now in, which means we have hard proof of how much even a static cache would benefit and can adjust those performance tests accordingly.

I also tested a repeat test run, but those are missing the chrome container: https://git.drupalcode.org/issue/drupal-3438070/-/jobs/4781741

I removed one extra request.

Locally, the test passed without any frontpage request and no sleep but on CI it didn't. Unsure how relevant it is to warm up the regular front caches and asset stuff at all. I tested once by going to /jsonapi overview instead, but that for example removes the path prefix lookup for /jsonapi and I kind of like having that once in there.

Changing the subscriber to be after routing/dynamic page cache exposes the problem again for things that are not part of dynamic page cache.

This already happens on HEAD, but there it still hits the data cache. OpenTelemetryAuthenticatedPerformanceTest exposes this perfectly. Before the last change, I was able to move 2 cache lookups from data to bootstrap because it has the cacheaable list there (This is the isFrontPage() check in LanguageBlock, so very similar to redirect module)

I think this is doing too much at once, but I'm not quite sure yet how to cut it yet. Will try to work on an updated issue summary when I have more time, but essentially, I guess we should keep the data cache for non-optimized cases, then we should mostly be able to keep the status quo. And we can either optimize regular requests and ignore json:api for now, or optimize that. And considering that we now have a lot more blocks being rendered outside of dynamic page cache, I think it makes more sense to focus on that. A separate issue/request subscriber could still add jsonapi routes on jsonapi requests, bonus points for figuring out how to not add the regular ones.

Remaining is also BC (added a new method to an interface for now for the unit test) and naming (RoutePreloader no longer preloads routes) things to figure out.

Merging in the json api performance test, as expected, getting 7 or so extra uncached queries atm.

Testing my idea of storing routes per format, instead of only html. on umami, that results in this route count after enabling rest and jsonapi:

so, 140 HTML, 225 for jsonapi. And that's not a huge amount of bundles and field. jsonapi creates a 2 routes for every bundle plus 2 for every reference field/relationship. Seems like that should be possible to do using parameters, but that's a different topic. I didn't enable any resources for rest.

I'm a bit surprised that I didn't see any ajax/js formats, I guess those routes are using html, so I also assume that currently they are not preloaded as the request format then afaik doesn't use HTML. Maybe we want to unify them to HTML? So many scenarios where performance test could be interesting (views ajax, form ajax?).

Anyway, implemented that, and expected the number of queries to go down on the second cached request then. Except they didn't. The reason is that jsonapi is faking support for the request format. See \Drupal\jsonapi\Routing\EarlyFormatSetter. But with my request event priority change in this issue (to fix route caching for lookups coming early from redirect module), it doesn't yet see that. Reverting the weight change for now gives me the expected result. All the data cache lookups now shift to bootstrap lookups. Whether that's really worth I'm not quite sure.

As far as the entity API is concerned, *create* is the operation that creates a new entity object, not when its' saved. For a form, that happens kind of the first time you access the node add form. There are some edge cases form state cache, so on a form without ajax and without permission to set a created date, you _might_ actually get it to use the time of the submit, haven't tested.

I understand that the form workflow changes a bit how the API works due to storing the value and the created entity, but it's not really a form and even less a node.module issue.

I get that this is in some cases unexpected, but changing this, especially after it has worked like this for 10 years, is also going to run into some edge cases. What about previewing an entity before it's saved, what would you expect there for something that displays the creation date? Also, as soon as drafts and workflows are involved, you're likely more interested in a published time than a created time anyway in many cases? Meaning, changing this would still not really fulfill expectations that people might have?

Having a standard setting for this would allow redis to adopt this as well, by respecting this with the TTL (which redis currently doesn't set by default: 🐛 TTL handling broken, always permanent Needs work )

I'm going ahead and closing this as a duplicate, see all the issues that were recently worked on in 11.2, specifically the children of 🌱 Optimize render caching Needs work .

Blocks can now force a lazy builder placeholder, and we have the ability to bulk-load them from render cache, we have CacheOptionalInterface and either 📌 Make language switcher block cacheable Postponed or a spin off of that (if it doesn't happen there, we could reopen this) will implement support for respecting that in BlockViewBuilder.

No updates since 2016, so it looks ike at this point, nobody really considered this a problem?

Created a second MR that uses a forced placeholder and the CacheOptionalInterface. This makes it kind of similar to HEAD, no impact on performance tests, but unlike max-age 0, this doesn't bubble up and shouldn't break page caching once that looks at that. It could maybe also use 📌 Allow the messages block to skip placeholdering Active once that's in, but would require an alter hook I think to set it on the right level.

Trying to go back to keeping it cacheable but using the new forced placeholder ability. That kind of has the same benefits to cache, although that's mostly FastChained lookups anyway. twig debug shows that it's pretty fast, but it can get more complex with multiple languages I suppose.

I could also experiment with CacheOptional + placeholder and maybe also skipping big pipe (

@smustgrave: We're not fixing those issues. This just changed the render context/time so we're not longer applied by it. In that mode, it also can't be reverted, because that was the whole point of this change. That said, the new placeholder approach might again change how this works, we'll see, didn't run that test.

Note: cache_control_override, at least in it's current from, does _not_ deal with page_cache, see

There was lot of activity on 🌱 Optimize render caching Needs work and related issues in the last few weeks and months. That said, I don't think that is actually blocking this at all, as it's just optimizing (the scope of that meta changed a few times).

So removing it and reducing pp, although i have no idea what the correct number for that is. The most obvious one is still the language block, but I'm working on that.

> Things setting their max-age to 0 to declare themselves as uncacheable

> Would love to hear if people agree with the splitting up of this problem and if so, whether or not the first (nonzero) part should happen in this issue and the zero part be handled in a follow-up or if we should keep the zero part here and split out the nonzero part.

In fact, our custom response subscriber does exactly that, it ignores a 0 max-age. But we've also been using the language block patch for years. The problem is that in this scenario, anything setting a max age 0 will remove the ability for anything else to set a finite specific cache max age. Most multilingual sites have a language switcher (or multiple) on every page, so this won't do anything practice for them.

I recommend rebasing this so we can see the current effect this has on the performance tests.

It's not the process of rebasing, that's fine, although I'm confused by the phpstorm UI and prefer to just edit manually.

In this case, both issues shave off 10+ cache lookups and when combined, it will improve further, but not just the sum of both, so we'll need to rerun the performance tests to get the numbers. But that's necessary in either direction of them getting in.

It's really just more logical for me to do the other issue first, it's a smaller, less complex change, it happens earlier in the page flow and getMultiple() will be much more involved then (as in, more lookups at once), making us more confident that the change is correct.

But if this happens to get RTBC and committed first then perfectly fine too.

(mostly just adding the parent issue)

We have some rules around skipping tests if the fix is straightforward and unlikely to break again. This is just a safety guard against code that was not properly updated to D10+ to provide a more helpful error.

Testing it would require that we add an invalid test implementation of a field type and that seems a lot of trouble for this.

Lets try :)

Unsure about the helper functions. Unlike some others, they are not underscore-prefixed nor are they hooks, so technically they are an API. in https://git.drupalcode.org/project/drupal/-/merge_requests/10999#note_45... I proposed to keep them but deprecate them for D12.

Unsure. form serialization has changed, we now basically only start to persist them on the first ajax request. So I think that basically makes this a non-issue unless those forms use ajax.

Lets just close this unless someone has proof that this results in a measurable improvement?

Replied.

Yeah, it's tricky.

FWIW, the project page explicitly talks about solving the problem of the referenced core issues, which it does in fact not do yet, it does something different. So that seems a quite weird. This issue then actually makes it do what it says there, but it still does the max-age, which might result in quite unpredictable results.

The max max age check is already there above, so yes, I think the extra check can be removed.

Still, the problem IMHO is the combination of the two headers and applying the same rules to them.

For external/proxy caching, having a maximum max-age makes a lot of sense, most sites I think don't really have any means of cache tag based invalidation, definitely not in browsers ([#/3350593]). But applying that same max-age on the internal page cache could result in considerably lower cache hit rates. Then again, it will only run if there is a specific max-age set that's not permanent. permanent is still default core setting.

It all seems quite unpredictable. I'm not a maintainer, I just ended up here because I've been telling people in the past, including in those core issues to use this project, only to realize that it doesn't actually do what I expect.

As you said, it probably also depends on your use case and how you handle caching. For us, that's basically two things:
1. We set a low max-age (like 15min), which means that every 15min or so, a given page needs to be revalidated against the internal page cache, which caches by default forever.
2. For a few specific pages, we want to have limited and lower max age/expire in the internal page cache, for example event listings.

So this currently, with and without this issue, isn't a good fit for us and I'd assume also many other small to medium sites that don't have a custom reverse proxy with invalidation support.

I think the most sensible thing for this project would be to have 3 different on/off settings for max-age, s-max-age and expires, each with it's own separate min and max. And for a consistent result, I'd then always enforce the given max-age setting, even if the cacheability metadata says permanent. Then you can customize it to fit your use case.

Pushed some docs improvements, I think that's sufficient? Since hook_theme() docs/example is non-OOP, I can't really put `static::class . ':preprocessList'` in there, but I mentioned in the docs above.

FWIW, I disagree with considering the max age and that is a change to how core handles this.

Core specifically only sets the max age setting on the response and the internal page cache is forever. If you have no reverse proxy or something like this then it is common to set cache.page.max_age to a low value, because you have no means to invalidate it.

The UI label for the core setting is specifically " Browser and proxy cache maximum age".

Then again, our custom implementation of this only sets the Expires header and doesn't touch max-age at all. This is also how I imagine the core implementation will behave, it will consider cacheability max-age for the internal page cache, without altering the Cache-Control header.

Rebased and squashed the commits together to make future rebases easier.

A test would require another hook that returns nothing. I'm not sure that's really worth it.

I'm also not sure we actually want to support this. we could also explicitly deprecate a non-array return value.

either way, leaving at needs work to update the second place

getConfig() is still broken. It does not return a Config object, it returns the ConfigFactory because the condition there is never TRUE to go inside the if condition.

It works because it does not have a return type, which I'm not sure why phpstan doesn't complain a about.

The tests pass because what actually happens is that it returns a new config object for the non-existing maintain_index_table config and that evaluates to true. We have no tests that disable this flag.

drop the if, add an explicit return type, and just make it a one-liner to return that config. Since that's the only config key we care about, I'd actually suggest to rename the method to something like shouldMaintainIndexTable() and directly return a boolean from that.

Created is a specific field type that sets the default value: \Drupal\Core\Field\Plugin\Field\FieldType\CreatedItem. The node form uses the default widget for that field type, no special logic.

And ChangedItem extends from CreatedItem and just extends that to then also update it on save.

IMHO that's by design. All fields have the capability to have a default value and that's set when the entity is created, not actually related to the form at all.

Which happens to make it a duplicate of the *even older #29338: Hide Promoted/Sticky fields by default in Form display → , lets consolidate and close this in favor of that?

To be clear I agree it's a mess, but I don't think this issue will help with changing that.

For starters, It's factually wrong. What this method/API does is control whether or not the query access alter hooks are invoked or not. Whether or not you have those is an entirely different question. And that's I think is covered with existing issues.

For context, one useful resource are selection plugins, beside some entity type specific settings, the main reason they were moved into core together with the entity_reference module is access control. See \Drupal\node\Plugin\EntityReferenceSelection\NodeSelection and \Drupal\user\Plugin\EntityReferenceSelection\UserSelection for two examples.

Similar problems with views, which has custom hardcoded access plugins that don't work with some newer permissions from content moderation and so on.

The goal would essentially be to move that one layer deeper directly into entity query access. But between that and the node grants API and BC and very little interest by the general community, it's really hard to make meaningful improvements in this space.

will rebase this after 📌 Try to preload cache tags in cache getMultiple Active

Merge request created.

This results in a fatal error if a field_widget_third_party_settings_form hook doesn't return an array, such as entity_browser_entity_form_field_widget_third_party_settings_form(), which conditionally adds something only for a certain plugin.

created a follow-up: 🐛 entity_browser_entity_form_field_widget_third_party_settings_form hooks can no longer return NULL Active

berdir → created an issue.

Just like node grants, it only does something if you have modules implementing those query alters. It does something out of the box for commerce orders for example.

Yes, it's a problem, there are plenty of open issues about it, it's essentially about core not implementing its own access systems "for performance reasons".

Re #14:

In short, the whole config dependency recalculation stuff goes through things, it loads all config entities, including roles and language content settings (for example when you delete a translatable node type). Then it makes changes to those config entities in memory, for example removing the permissions to create/edit/delete nodes of that node type. Then it goes through them and starts saving them. While saving the language_content_settings config entity ( think that happens before the roles, so they are still in memory), content_translation module now again loads the roles, makes another change to them, saves it. And then we save the role config entity again later on.

This whole thing only works because roles are statically cached, so Role::loadMultiple() already gets the altered versions.

I also think this really should be using override free load, see 🐛 Prevent saving config entities when configuration overrides are applied Needs work and 🐛 UserPermissionsForm should not use overridden permissions Needs work but there's a good chance that the whole thing will then break because they will then no longer share the static cache. At least not until we do the same in the config dependency management (which we should).

> Is it weird that the bot is picking up PHPCS issues that the build is not?

No, this is just a very new rule, was not enforced before.

I added some docs, changed the theme.preprocess service, restored and deprecated the template_preprocess_HOOK() functions and also updated all @see references in templates for the converted ones. That blows up the diff stats quite a bit (64 files, + 688, − 394), since all the function docs are no longer removed.

Also had to reflow several comments due to the new enforced phpcs rules.

FWIW, Hook is a class that exists in 11.1, there are likely no subclasses yet, but changing the type like that is a technically a BC break. So will need a decision on whether that's important enough to warrant that or this is a won't fix.

It's doing exactly what it says. It uses the title of the current *page*. Page in this has nothing to do with nodes, it's just about the current request. In pathauto pattern, *always* use entity patterns, not something else.

fixed new phpcs rule.

smustgrave → credited berdir → .

I adjusted the navigation test. I also implemented my idea about !isMethodCacheable(). In regards to queries on standard profiles, it's a bit a wash, one query saved, 7 extra cache gets and therefore also 7 extra cache tag lookups. But menu blocks on real sites might have huge menus and so on that would be expensive to calculate and several of the cache tag lookups should go away again with 📌 Use a single per-theme cache tag for block config entities Active (will pick that one back up once the current set of optimization issues are in)

agreed about this being done, but tests will either way heavily conflict with ✨ Optimize placeholder retrieval from cache Active , no matter which gets in first. could in theory also be this one I suppose.

If you look at that, it also results in major reduction of cache get lookups. Right now, getMultiple() support is by far not as effective as it could be without that change because most single placeholders are still first requested with a single cache lookup and fill up the static cache one-by-one. By combining the two issues, all those placeholders will only be a single getMultiple(), resulting in a larger improvement than either issue can achieve on its own.

to me it makes sense to do the other issue first, it seems more logical to optimize that first, but in practice the result is the same, after one of those two issues in, the other one will need a major rebase and rerun those tests to get the new combined numbers.

I expect the gains from getMultiple() will only become visible once ✨ Optimize placeholder retrieval from cache Active is done, which itself is soft-blocked on 📌 Try to preload cache tags in cache getMultiple Active to avoid ending up with more cache tag lookups in some test scenarios. Lets focus on that order?

Rebased.

I don't think I'm the right person to answer this, but this work does include some tests that were not added as part of the other duplicate issue, namely limited testing for cold and cool caches as well as listing out in full the expected 'CacheTagGroupedLookups' for several tests. Are these useful additions?

Yes, but the issue that was committed is just one of multiple issues in the meta parent. If you check the child issues listed in the sidebar, there's a dedicated one for each method basically.

I think it's probably OK to merge extending the existing methods into issue, but then we should comment on the 2-3 that are now duplicate and close them to avoid further duplicate work. I also updated this to make it a child of that meta.

Has been a while and there have been lots of improvements and updates on how we assert, especially around cache lookups

Does hot/cold/warm really add a benefit here? hot is just testing the access denied page as an anonymous user?

I'd rather focus on warm caches and only with an authenticated user.

What I think could be useful instead is actually saving an entity, including an update. Some overlap with 📌 Add cache invalidation performance test Active in that regard.

berdir → created an issue.

One option to change this is to move it to an authenticated user and save the node through the UI. That should both avoid this weirdness and allow to a performance test on saving a node, which would be interesting to track improvements around that.

And that will increase further with ✨ Optimize placeholder retrieval from cache Active , makes sense to me to get this in first, so that the other issue then has fewer regressions. Didn't test that yet, but fairly certain it will help with OpenTelemetryAuthenticatedPerformanceTest.

It should be, nothing changed except performance metric conflicts. One reason I didn't set it back myself is that there are a lot of performance issues right now and it's not as important/big of an impact as some others, but any improvement helps...

Rebased and test assertions updated, green again.

One more performance test updated. I didn't investigate too closely what's going on there, that one bumps the cache tag lookup from 4 to 8. sounds like that previously had those blocks cached as part of the dynamic page cache. Probably because this test doesn't empty the render cache after logging the user in, so a bunch of blocks are already render-cached, which is exactly what this is about. 📌 Try to preload cache tags in cache getMultiple Active should help with that and reduce that again by 3-4?

Pretty challenging how we have so many overlapping performance issues that all have non-trivial, overlapping impacts.

Ok, now that block placeholders are in, we're seeing major improvements here that more than make up for the regression introduced by that issue. Not 100% sure it really makes sense to split this from ✨ Optimize redirect chain retrieval in VariationCache Active but probably yes, just the performance test impact overlaps are a bit painful.

Now that this removes those initial cache get requests, the benefits of the other issue will be be limited in the current implementation. However, the combined improvements should be even bigger once the other issue supports getMultiple() properly.

I also tracked down the change around the login test changes. It's... interesting, especially around the block login one.

What's happening is that we now no longer do a cache lookup for placeholdered blocks. However, on POST requests, we actually don't put placeholders in but always immediately render them. Combined with the change a while ago about being able to render cache elements on POST requests, we are actually able to return a render-cached menu block in this scenario, but not anymore with this change.

We could keep the current behavior in this scenario, by adding an additional check for !isMethodCacheable() so that we still use the render cache one-by-one on POST requests, assuming that if we start rendering blocks, we're likely going to be processing a form in one of them.

I was wondering if skipping placeholders on POST request is still the right to do but the short answer is yes. We can not process a form in bigpipe after the response has been sent.

I'd be ok with moving forward. there's still the question about the references in the template files. any preference in regards to updating or removing them? I'll try to ping Theme API maintainers over this issue in general and specifically this.

Started a new merge request, did runtime/update with an internal method that keeps $phase and only removed the install stuff. copied the install requirements from the old MR.

only did enough changes to make phpstan/phpcs happy.

Oh, i hadn't seen that you already started a change record. I updated and extend it:

* Made it clear that this this is the new format and it replaces template_preprocess() (no "can"). Once this is committed, the old style is essentially deprecated and deprecations will be added once core is converted.
* Switched to block as example, because nobody except core will have to deal with the ThemeCommonElements and theme.inc stuff like that.
* Extended the example to include the hook_theme() definition in both cases
* Explained and linked the CallableResolver and what's supported and what not.
* Added example on how to provide BC with older core versions
* Adapted the breaking changes section, there is only a single template/initial preprocess (for a given theme hook, now that template_preprocess() is deprecated), you can't reorder them. while you could technically move them after other preprocess callbacks, I don't see a scenario for that, that would definitely cause problems. I strongly assume the main use case for that was just explicitly defining it for dynamically defined theme definitions.

> let's convert enough in this issue to prove the concept,

Yes, that was the idea, convert a few different examples, including DI. One example more that could be considered would be one in an include file, so we can "prove" that we can remove and deprecate the file key then afterwards. but I think that's pretty obvious?

Remaining steps here:

1. Decide on BC. I removed them for now to make the review easier, but I think we want to keep at least the ones in theme.inc as a BC layer, there are a bunch of calls to them, for example the one in theme.inc about the maintenance (and then the install steps page chained). Not sure about the module ones, but there are far less than module hooks, so doesn't hurt too much to keep them. Plus, the BC layer is implicit and free, if there is an initial preprocess callback defined, we skip template_ discovery.

2. Remove references. Tons of twig files reference their template_preprocess_xy() function. We can either update them or remove them. I'm unsure what the value of that is and it's duplicated a lot and all the copy pasted templates into contrib/custom themes will be out of date now. So future changes might invalidate them again.

3. Change Record/documentation.

I'm fairly certain this is resolved in 11.1+, yes. Moving back to 10.5.x, there's still an option to commit there as a workaround. Instead of rerolling patches, please createa merge request, specifically against 10.5.x.

Worked a bit more on this, introduced a ThemePreprocess service and moved a bunch of template_preprocess() callbacks into that, specifically those that use services, so we can see a bit more how that works with DI.

Doing all here is too much I think, especially since some of them should probably also live in different compoments, for example field templates could move to Drupal\Core\Field, either in a separate service or an existing one if something fits.

Also noticed a node dependency in template_preprocess_page(), that should probably move to node module, no reason for it to be there, but also kind of out of scope.

And, had to shuffle the bubbling stuff around a bit in ThemeManager, noticed while testing the installer that it wasn't called if there was only an initial preprocess and nothing else.

We have several test entity types with string ids now, there are likely gaps in the test coverage for them, but I don't think still having this issue open will help with closing those, so lets close it.

Looks like assertEqualsCanonicalizing() isn't as nice as I thought. it ignores order, seems to even sort the array in fact, so that would be quite annoying to resolve.

An option might be to just compare the whole thing as as string?

$this->assertSame(implode("\n", $expected_queries), implode("\n", $recorded_queries));

gives me the following when switching order of two queries:

--- Expected
+++ Actual
@@ @@
 SELECT 1 AS "expression" FROM "path_alias" "base_table" WHERE ("base_table"."status" = 1) AND ("base_table"."path" LIKE "/node%" ESCAPE '\\') LIMIT 1 OFFSET 0
 SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "core.entity_view_display.user.user.compact", "core.entity_view_display.user.user.default" )
 SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "filter.format.restricted_html" )
+SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "system.image" )
 SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "user.role.authenticated" )
-SELECT "name", "data" FROM "config" WHERE "collection" = "" AND "name" IN ( "system.image" )
 SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "theme:stark" ) AND "collection" = "config.entity.key_store.block"

could have an assertQueries() method on the PerformanceTestTrait that does that? Can open an issue if you think that's useful.

Fixed the last non-performance test fails now I think. That one was interesting, it was reverting an old revision that was the the default revision back then to the new current revision. The logic here was to invalidate the loaded revision, but that wasn't actually the previous default revision. I extended it to also invalidate $entity->getOriginal()->getRevisionId() which works in this case because the revision we revert to was the default, so the logic in \Drupal\Core\Entity\ContentEntityStorageBase::doPreSave() does not kick in and it does _not_ use loaded revision as source. But if you would revert back to a draft then that wouldn't work. To fix this for that (non-tested) scenario, we'd need to reliably know the previous default revision id.

I'm also not 100% sure what makes sense as getOriginal() but I think it is in fact the current default revision, because that's what we're changing from.

Or, we drop this pretty complex optimization logic to avoid invalidating all old revisions when saving an entity. This would get a bit more expensive specifically when we'd combine it with the option in #183 to remove the cache tag and explicitly delete all known revision ids. But you'd probably need many thousands for this to really be an issue.

We have \Drupal\Core\Entity\SynchronizableInterface since 2018 which is exactly this.

I've been working on "saving" the persistent cache and reducing the BC impact of this. There are a few interesting advantages of keeping the persistent cache, for example the optimization with loadRevisionUnchanged() that allows to load a revision from static cache and only bypass the static cache, which is very useful when saving an entity, and also just loading the current revision which is what's going on with getActive().

* I don't see a use case for invalidating a specific revision as an API. The cache should be self-contained, basically the only reason for external calls to resetCache() are tests and we've mostly removed that too thanks to automatically invalidating the memory cache.
* This allows to remove the default_revision_for cache tag, we should always have the entity ID available and then we can delete that from the cache directly.

The remaining cache tag is now revision_for.., that still shows up a lot in the performance tests, because of the optimization to also put the default revision with its revision ID into the cache, but that means we need to check the state of those cache tags on cache set.

I plan to look into either not doing that anymore for now or alternatively remove the cache tag. The number of revisions for an entity is a known quantity. So in cases where we need to invalidate them, we can query the ids and then invalidate the cache. I think that's not necessary that often, basically just when an entity is deleted?

Looks like the most recent changes broke something, will look into that, off for now.

the CI job runs phpstan like 3 times with different output formats and arguments, so I don't think we can reuse that.

Rebased, just creating two nodes now with a get inbetween, then the route cache is filled.

Closing this as won't fix/duplicate of 📌 Deprecate CacheBackendInterface::invalidateAll() Active , which deprecated invalidateAll(), the other two can be useful and aren't really a problem to implement.

This would definitely be useful. I've added ddev commands for now:

$ cat .ddev/commands/web/phpstan-generate
#!/usr/bin/env bash

./vendor/bin/phpstan analyze  --configuration=./core/phpstan.neon.dist --generate-baseline=core/.phpstan-baseline.php

And a phpstan without the --generate-baseline.

Test fails look random, don't have enough permissions to retest, but changes look good. And yes, query changes weren't so bad since a large chunk just moves down. I already had to reroll 3 or 4 issues at this point that make changes on the list, so we're going to need a lot of rerolls around all those performance issues.

I might open a separate issue to change the query list asserts to use $this->assertEqualsCanonicalizing($expected_queries, $recorded_queries);. From what I've seen, that results in output that's much easier to parse as it doesn't compare the array keys. but I need to check how it deals with order changes and so on.