ChainedFastBackend doesn't set 'expires' and 'tags' when writing to fastBackend

Interesting.

Expiration yes, I can imagine that could be a problem. But I'm actually not sure about cache tags.

Any set or delete of single items or cache tags results in a complete invalidation of the fast cache, so why bother with checking them?

Yeah, should have read that.

Discussed with @WimLeers, as we apparently do not bother to send invalidated/deleted cache tags to the fast cache backend (which does make sense because we invalidate anyway), we should not save the cache tags, as that will just result in unnecessary processing in AcpuBackend::prepareItem().

But we need to document why.

Yep #3/#4 looks right to me as well, but agreed we need explicit documentation on why it gets treated like this.

#4: I found this bug when implementing the cache datacollector for the Webprofiler module, If we want Webprofiler to collect and show cache tags we need to save them also to the fast cache backend

There's a test patch in #2304413: Add generic cache backend unit test for ChainedFastBackend.

I think that the changes to ChainedFastBackend::getMultiple() are not necessary. The fast cache backend should already check if an item is expired itself, that is part of the API contract and I don't see why we can't rely on that.

To #8, why do you need the cache tags in the fast cache backend? As mentioned elsewhere, we immediately invalidate the fast cache backend on any cache tag invalidation, so adding them just results in pointless cache tags processing on getMultiple(), as they will never be invalidated...

Here's a patch without the expiration stuff and unified and documented the tags stuff to never set them on the fast backend.

I need them into the fast cache backend only for profiling purpose, not for cache expiration. If this cause a performance regression I just have to replace the fast cache backend service with one that saves cache tags when Webprofiler is enabled, no worries.

RTBC for me

Working on integrating #2304413: Add generic cache backend unit test for ChainedFastBackend and providing test coverage through that, seeing some issues with that...

Let's open a normal task to discuss tags in the fast backend. I doubt it would be a measurable performance regression (it's supposed to be a fast backend after all), but it's also right in the critical path and we've seen some strange numbers with unserialize() and APC in other issues.

Ok, this turned out to be a bit more complicated than I thought bit I finally figured it out, thanks to @alexpott.

I merged in the patch from #2304413: Add generic cache backend unit test for ChainedFastBackend and looked for expiration tests in the generic cache backend, so we have real tests for the expiration, with an actual implementation for the fast backend, to proof what I said above.

Turns out that we *do* have expiration tests ther but it passes already for ChainedFastBackend (as visible in #2304413: Add generic cache backend unit test for ChainedFastBackend) because it only tests for immediate set()/get(), which already works fine, it's only on the write-through after invalidation, that it was not working. So extended the new test there with a specific scenario that writes another cache entry in the middle for the chained fast backend.

That test also currently expects cache tags to be returned, so I reverted that part from my patch for now, we can look into avoiding that in a non-critical issue I think.

When running those tests however, I had weird random fails. Took me a long time to figure out out, but I think I finally have and I had to make the following changes:
- mark as outdated needs to happen *after* we update the persistent cache, so that we can be sure that other clients will get the correct data when they update their fast caches
- The actual problem I think was that that the cache created time is milliseconds and the last write was microseconds, and due to rounding, it sometimes happened that the millisecond was rounded up and created was then after last write in the comparison. So I changed that to round, which then resulted in even more fails is was then always the same millisecond. So the final change is to put the last write timestamp 1 millisecond in the future, so we possibly have to re-fetch a few cache entries from the persistent cache, but we can be sure that there is never wrong data returned.

Also, don't forget to credit @olli for his work in #2304413: Add generic cache backend unit test for ChainedFastBackend.

Forgot to re-enable the new test.

Right, so I was confused.

The test can not work, because the consistent backend will obviously never return an expired item that we could put into the fast storage, so we can not reproduce this in a kernel test, it has to be in the unit test. Which did not actually properly check for this yet, so I removed the custom test method and instead ensured in the unit test that we're calling it as expected.

Ah, forgot one thing, we discussed the order of set/markAsOutdated(), and the best approach there is to write to persistent first, then mark as outdated, then update our local fast cache. That is safe and we don't already invalidate or own set.

Added @olli to the commit credit for the dupe issue. This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 106e3d4 and pushed to 8.0.x. Thanks!