Problem/Motivation

Bug because this is a performance problem.

See #2765271: Rationalize use of the 'discovery' cache bin, since it's stored in the limited size APCu by default for background and data. token info is in my case ~1MB of data, storing that in apcu is not a good idea. We also don't need it on most requests.

Proposed resolution

Move all entries to the data cache bin. Do so by changing the service definition for the token service and pass in cache.data instead of cache.discovery.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#13 move_token_info_cache-2824548-13.patch659 bytesfaline
#8 move_token_info_cache-2824548-8.patch775 bytesfaline
#3 move_token_info_cache-2824548-3.patch656 bytesginovski

Comments

Berdir created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +php-novice
ginovski’s picture

ginovski commented
Assigned: Unassigned » ginovski
Status: Active » Needs review
StatusFileSize
new move_token_info_cache-2824548-3.patch656 bytes

Changed to cache.data.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Tested #2824547: Change ViewsData to use the default cache bin instead of discovery and this together:

Before:

mysql> select count(*), SUM(length(data)) / 1024 / 1024 as sum, max(length(data)) / 1024 / 1024 as max from cache_discovery;
+----------+------------+------------+
| count(*) | sum        | max        |
+----------+------------+------------+
|      425 | 9.20685387 | 3.06007671 |
+----------+------------+------------+

After applying both patches, clearing cache and visiting a few pages to make sure the caches are filled again:

mysql> select count(*), SUM(length(data)) / 1024 / 1024 as sum, max(length(data)) / 1024 / 1024 as max from cache_discovery;
+----------+------------+------------+
| count(*) | sum        | max        |
+----------+------------+------------+
|      354 | 4.78700542 | 0.35403728 |
+----------+------------+------------+

Maybe a few plugin types haven't been discovered yet, but I guess most of the things are there.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Yay!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Hmm so I'd actually put this in cache.default

cache.data is supposed to be for relatively unbounded, non-render stuff like the path alias cache. In case for example you wanted to use two different LRU cache bins, one for bounded and one for unbounded items.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Needs reroll
faline’s picture

faline commented
Assigned: ginovski » Unassigned
StatusFileSize
new move_token_info_cache-2824548-8.patch775 bytes

Change to cache.default

felribeiro’s picture

felribeiro commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: move_token_info_cache-2824548-8.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I added an additional test, I thought testbot just was having a bad moment there. But then I saw the problem:

+++ b/core/core.services.yml
@@ -1,4 +1,4 @@
-parameters:
+  parameters:

The indentation of this has been changed, and is what's causing problems.

The last submitted patch, 8: move_token_info_cache-2824548-8.patch, failed testing.

faline’s picture

faline commented
Status: Needs work » Needs review
StatusFileSize
new move_token_info_cache-2824548-13.patch659 bytes

Thank you Wim Leers.

Correcting the previous patch

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

Yay, thanks!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.3.x, thanks!

  • catch committed d3ba24a on 8.3.x 
    Issue #2824548 by faline, Ginovski, Wim Leers, Berdir, catch: Move token...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Don't we also want to commit this to 8.2.x, to mitigate problems there too?

catch’s picture

catch
he/him
Primary language English
commented

I thought about that, but it means that for example putting cache.discovery in the memory bin while developing will no longer disable caching for tokens and you'd have to add cache.default to your local settings too. If the cache item was actually failing to write to apcu on people's production sites then it'd seem definitely worth it, but afaict the parent issue has switched to just rationalising what goes in apcu - which is great and will help sites that are over or near their apcu limit, but there's some degree of trade-off.

Status: Fixed » Closed (fixed)

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

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +8.3.0 release notes