Problem/Motivation

Slightly related to #3583505: Use Symfony PhpDumper instead of a serialized array container structure in that reviewing that gave me the idea for it.

We have multiple chained-fast bins in core - cache.bootstrap. cache.config, cache.discovery, cache.routes

For each bin, there is a separate last_write_timestamp lookup which always has to hit the persistent backend.

It would be possible to merge the bins into fewer bins, but chained fast is only effective when bins are written infrequently, so the less bins there are, the more writes there are likely be and the more cache items need to be fetched from the persistent backend. This means we tend to have a choice between more bin invalidation lookups vs. more individual cache item lookups.

If we centralise the timestamp lookups into a single chained fast backend, we go from one consistent lookup per-bin-per-request to one per request regardless of the number of bins, the individual timestamps will come from APCu when valid, from the consistent bin when not.

Steps to reproduce

Proposed resolution

Yo dawg I head you like chained fast backends so I put a chained fast backend in your chained fast backend so you can cache chains faster

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3611402

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

catch created an issue. See original summary.

catch’s picture

Talking to @longwave in slack one issue with checking things once at the beginning of the request is it brings forward the point where the semaphore could start to be stale - e.g. if a cache bin is invalidated after the semaphore is retrieved then the request wouldn't notice, but it might if the cache bin was looked up a bit later.

For chained fast we set the timestamp into the future when it's invalidated to account for this (at least for a normal-length web request), so we could do the same with this - use a timestamp and check it's in the past as well as it matching what's stored in APCu.

catch’s picture

This can't work - the items in the chained fast backend can be older than the bin invalidation, that's why we have to check it. So if they're older than a global semaphore is, we'd still need to check the individual bin invalidation timestamp or look them back up anyway.

catch’s picture

Title: Add a centralised semaphore to skip some chained fast checks » Add a centralised invalidation for backend for chained fast checks
Issue summary: View changes
StatusFileSize
new66.71 KB

#3 was correct that the original idea won't work, but then I realised it can work if we keep the individual bin invalidation timestamps in APCu too.

And then - what lets you store things in APCu and a consistent backend, with a single lookup allowing multiple keys to be fetched from APCu - chained fast backend!!!!!

catch’s picture

Status: Active » Needs review

MR is up.

The only alternative name for CoordinatedChainedFastBackend I have is ChainedFastChainedFastBackend.

amateescu’s picture

ChainedFastChainedFastBackend

This is what I'm here for! :)))

catch’s picture

Performance tests look superficially worse but show it working - for each chained fast backend, there's now a look up against the invalidation backend bin, which should hit APCu (+ 1 check against its own consistent backend).

Haven't added explicit test coverage here yet, but hoping for a green pipeline on existing tests.

catch’s picture

Title: Add a centralised invalidation for backend for chained fast checks » Centralize chained fast bin invalidations to reduce consistent backend lookups
catch’s picture

So what happens with this is the bootstrap, config, discovery and routes chained fast cache bins in HEAD each have to look up last_write_timestamp in their persistent backend once per request.

With this change, we move the last_write_timestamp checks to a separate, single, chained fast bin.

That chained fast bin will look up it's own last_write_timestamp entry in the consistent backend, and then it uses the chained fast logic that we already have to determine whether it needs to hit the consistent backend for the other bin timestamps.

The result should be instead of 4 persistent backend lookups, one persistent backend lookup + 4 APCu lookups.

If there's been an invalidation, it will be 5 persistent backend lookups instead of 4.

catch’s picture

Issue tags: +Performance
catch’s picture

@longwave's point in #2 is still correct but not sure it makes a lot of difference in practice - anything which fetches something from cache just before it's invalidated then static caches it has the same issue, this increases the window for that slightly.

With the discovery and routes caches nothing writes to those dynamically anyway as such - they get fully invalidated then repopulated.

The bootstrap cache is more dynamic, but is retrieved very early in the request anyway so the timing would not be much different.

catch’s picture

StatusFileSize
new403.4 KB

Since this doesn't show up in performance tests, because we log cache operations at the level of the bin rather than the individual cache implementations, I profiled the front page of the standard profile, with page_cache module uninstalled and logged out - e.g. a dynamic page cache hit.

Found one bug - with a return type hint of float we need to check for missing invalidation timestamps against === 0.0, added the return type hint to the parent class too since it's protected for consistency.

This reduces database cache gets for that dynamic page cache hit from 14 down to 11. This is because we get (at least) routes, config, discovery on dynamic page cache hits.

catch’s picture

StatusFileSize
new399.95 KB

Missed the 'before' screenshot.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new1.92 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

catch’s picture

Status: Needs work » Needs review
needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new91 bytes

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.

catch’s picture

Status: Needs work » Needs review

Rebased.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new98 bytes

The Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. 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.

catch’s picture

Status: Needs work » Needs review

Rebased.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new98 bytes

The Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. 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.

catch’s picture

Status: Needs work » Needs review

Rebased.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new91 bytes

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.

catch’s picture

Status: Needs work » Needs review

Rebased.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new91 bytes

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.