#Performance

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.

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.

This needs a reroll

Also drupal_static is being deprecated (see #2260187: [PP-1] Deprecate drupal_static() and drupal_static_reset() → ) so we should not add new usages of it. This would require moving node_access_grants into a service and deprecating the function.

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

This still exists in the OOP hook equivalent.

This is still overall a scalability issue in Drupal core, and there are some contrib approaches these days like https://www.drupal.org/project/entity_hierarchy → which uses nested sets.

Moving back to 'active' but converting to a plan.

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!

Very nice to see we’ve come so far that we can deprecate that! 🥳

I'm also seeing these in 'chained http requests' in Lighthouse with stock Drupal 11.x and the standard profile, so I don't think it's Drupal CMS specific.

…lora/lora-v14-latin-italic.woff2
…lora/lora-v14-latin-700.woff2

Changing this to a bug report because Olivero is implementing preloading but it's currently failing to preload what it needs.

The italic font appears to be loaded purely for the You haven’t created any frontpage content yet. although that wouldn't apply to Drupal CMS, but it could be two different reasons for the same thing. Couldn't yet figure out what's causing the 700 weight font to be loaded.

I'm doing this in 📌 Refactor system/base library Needs work

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.

@gapple if hook_rebuild() is being used for pure cache pre-warming (e.g. something that will still be populated on a cache miss) then yes it could switch to this.

The differences will be the drush parallel cache warming, and if there's a stampede, the ability to warm different caches at random during lock wait or while waiting for async database queries to come back. Whereas hook_rebuild() has to go linerlarly through whatever is put in it.

None of these are implemented yet but see related issues.

Would prewarming eventually be a replacement for hook_rebuild() when it's no longer @internal, or will they have different use cases?

Oh wow, for a second there my brain processed the assert as an if-statement. Left one more suggestion, but that can be fixed upon commit.

Following the steps in the issue summary confirmed everything is behaving as normally.

Rebased again.

If CachedStrategy does not find any hits, it will also return an empty array. Isn't that the same as what the test strategy did?
The only difference I see is that the test did not have SingleFlushStrategy as a fallback, which ends up returning all placeholders anyway.

Yes exactly - SingleFlushStrategy always replaces the remaining placeholders, which is why we can do the assert() to make sure there's none left to replace.

The unit test has no fallback, just a no-op strategy, and in that situation the 'removed' placeholder is not actually removed, but just ignored and not replaced, which we are now validating should never be allowed to happen.

So does that not mean that the check has to be !empty() to figure out that something was, in fact, not replaced?

No because the failure condition for an assert() call is when it returns FALSE.

https://www.php.net/manual/en/function.assert.php

Assertions should be used as a debugging feature only. One use case for them is to act as sanity-checks for preconditions that should always be true...

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.

It is not possible to do this, you can ignore placeholders, you can't remove them.

If CachedStrategy does not find any hits, it will also return an empty array. Isn't that the same as what the test strategy did? BigPipeStrategy can also return an empty array. The only difference I see is that the test did not have SingleFlushStrategy as a fallback, which ends up returning all placeholders anyway.

Might be my sickness talking but if I read ChainedPlaceholderStrategy::processPlaceholders() correctly, we reduce the set of $placeholders whenever a strategy returned some of them back. At the end of the loop we want $placeholders to be empty, no? So does that not mean that the check has to be !empty() to figure out that something was, in fact, not replaced?

Ideally, we don't go to the cache at all, which would be possible using a tag like CACHE_NOT_WORTH_IT or RENDER_CACHE_REQUIRED.

We use 'CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:form' to prevent caching of forms on POST requests now, so adding another 'special' cache tag would be consistent with that.

I think manual intervention is fine: we're mainly interested in:

1. Very high cardinality cache items that are usually inside something else, mostly to save writes/space. Like the paragraphs example but that already has a separate opt-out.

2. Very high frequency cache items that are as-cheap to render without caching, mostly to avoid unnecessary gets, like some of the 'site chrome' blocks identified by @berdir.

Render cache multiple get is now in core after 📌 Render the navigation toolbar in a placeholder Active , we should see if there's an example in core where this would help, and then try to implement it in the renderer.

catch → closed merge request !10799

That makes sense, just missed it, sorry for the false alarm.

Committed/pushed to 11.x, thanks!

Replied.

One question on the MR.

I think because of a small mistake.

I thought I'd missed the exclamation mark when you pointed this out, but it was actually fine, the problem was in the test.

The unit test sets up a 'placeholder removing strategy' that returns an empty array. It is not possible to do this, you can ignore placeholders, you can't remove them. So I just removed this from the data provider, it's testing something that's not supported and pretty sure the unit test would have choked on the new assertion since it was added.

Lets try and get to this before the bot attacks again :)

Actually all feedback appears to be addressed.

ChainedPlaceholderStrategyTest is choking on the new assert, I think because of a small mistake.

Americanizzze the code comments.

😂

Think I got to all the feedback, agreed on the more general assertion message.

Dug up the ::class docs for people stumbling upon this issue: https://www.php.net/manual/en/language.oop5.basic.php#language.oop5.basi...

Found some. minor things but looks good to me otherwise

Committed f6d07a1 and pushed to 11.x. Thanks!

nod_ → closed merge request !11439

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

Asked in slack about how to remember to add the type hint in 12.0.0 when the trigger_error is deleted. @larowlan suggested a new issue. The issue is 📌 Add type hint to RefinableCacheableDependencyTrait::addCacheableDependency Postponed .

Small question left

Straightforward rebase so restoring RTBC.

Rebased.

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.

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.

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.

Just following up on this, if still a valid feature could the summary maybe be updated?

If no follow up this could be eligible for closure in 3 months

Thanks all!

don't see any blocker why not to add UnchangingCacheableDependencyTrait, so views can still have max-age -1

If it breaks things, please report it here

catch → closed merge request !11121

Committed/pushed to 11.x, thanks!

We need a title and IS update here about exactly what we're proposing to change.