- Issue created by @catch
- Merge request !11135Issue #3493911: Add a CachedPlaceholderStrategy and use a placeholder to... → (Closed) created by catch
- 🇬🇧United Kingdom catch
Fixed @berdir's review point, and confirmed we need 📌 Create placeholders for more things Active to see a result
On https://git.drupalcode.org/project/drupal/-/merge_requests/7280/diffs?fi... you can see the cache tag queries go from 5 to 7, this will be two different sets of cache tags used for two different blocks.
On this MR, the cache tag lookups were 2 because it's already including @berdir's cache tag preloading logic. Without the change here, that would have gone to 4, but with the change, it only increases to 3 - e.g. two placeholdered blocks get their cache tags checked at once.
Or at least I think so, this is now stacked on a few too many different MRs that all make changes to performance tests, but if we can get some of the other MRs committed the effects will be easier to isolate.
- 🇬🇧United Kingdom catch
Postponing on ✨ Introduce a list of "common cache tags" to reduce lookup query amount Active .
- 🇬🇧United Kingdom catch
Still doesn't result in any changes in core performance tests without 📌 Create placeholders for more things Active but MR is green now.
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Looks good to me and looking forward to the results when we land the blocks issue. Setting to NW for performance reasons.
- 🇨🇭Switzerland berdir Switzerland
Something with the MR seems off, huge amount of changes, bad rebase?
How about we postpone this on 📌 Create placeholders for more things Active ? The issue title still says *Try to*, and so far, we AFAIK haven't actually seen any benefits yet. That should change with that issue, although apparently not as much as we'd expect, because that issue already manages to combine many blocks together. But it should then for example group the menu cache tags.
- 🇬🇧United Kingdom catch
Rebased again. I think postponing it on that issue is reasonable.
- 🇬🇧United Kingdom catch
Small but visible improvement now after 🌱 [meta] Placeholder-driven performance improvements Active
- 🇨🇭Switzerland berdir Switzerland
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.
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Re #10 at first it did that too for me but then when I refreshed it seemed fine. Must have had a hiccup in GitLab on my end, wouldn't set it to RTBC if it had a garbled history like it initially did.
Having said that, MR still looks great to me so RTBC +1.
-
longwave →
committed baa7ad08 on 11.x
Issue #3504902 by catch, berdir, kristiaanvandeneynde: Preload cache...
-
longwave →
committed baa7ad08 on 11.x