Updated: Comment #N
Problem/Motivation
Follow-up of #2182423: key_value_expire doesn't get reset and balloons the PDO execution time for form pages.
The referenced issue already improves the performance of the database key value expire backend quite a bit, but still does one a DELETE query on every page that writes, which now means every (ajax?) form.
Proposed resolution
Two ideas to improve it from the other issues:
* Add a random check, only execute in 10%, 5%, ... of the cases it is called.
* Remove the automated garbage collection, call it from system_cron(). It is a bit problematic as the collections are dynamic but the referenced issue already hardcodes the assumption that garbage collection works on all collections in the factory, we should invoke it with a arbitrary collection and call garbageCollection() on it.
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#14 | cleanup-keyvalue-on-cron-2186113-14-interdiff.txt | 1.87 KB | Berdir |
#14 | cleanup-keyvalue-on-cron-2186113-14.patch | 8.02 KB | Berdir |
#12 | cleanup-keyvalue-on-cron-2186113-12.patch | 7.52 KB | Berdir |
Comments
Comment #1
moshe weitzman CreditAttribution: moshe weitzman commentedI like moving garbage collection into cron.
Comment #2
sergeypavlenko CreditAttribution: sergeypavlenko commentedHello. I support the idea, move cleaning tables in cron. Cleaning tables at each page load may affect performance.
Comment #3
andypostThis destruction better to replace with invalidation before usage
Comment #4
andypostGC could work as
queueItem
about collectionComment #5
SpleshkaAlso support idea to use queue API, so on cron gc can drop expired data.
Comment #6
BerdirI'm not sure how that would work, who would add queue items? Don't see the benefit of doing that on cron, there's nothing that needs to be processed in batches and doing it on access would still result in additional INSERT queries during normal requests.
This is as easy as it gets, let's see if this breaks any tests, I guess so.
Comment #8
BerdirFixing the tests.
Comment #9
andypostThat's great! +1 RTBC if green
PS: About queue I supposed that cache cleaning in
system_cron()
could be serialized with queues, but once k/v ford not support tags this needs own issueComment #12
BerdirHow about that RTBC? ;)
Comment #13
Wim LeersLooks great! Only silly nitpicks:
s/garbageCollection()/garbageCollect()/
i.e. verb, not noun?
Either s/The/These/ or s/are//.
s/Cleanup/Clean up/
Comment #14
Berdir1. As discussed, this follows the naming pattern set by CacheBackendInterface and flood, all called in the same function.
2. I didn't get option 2, so made it These :)
3. You know where I have Cleanup from? :) From *two* similar comments in the same function :)
Comment #15
Wim LeersHah! :)
Comment #16
catchVery minor but do we really want to run system_cron() here or just call the garbageCollection() method directly? I don't mind but maybe worth a comment to explain what exactly is getting tested.
Comment #17
BerdirI think it makes sense to call it, because we want to test that actually running cron does it. Otherwise system_cron() would be broken and the test would still pass...
Comment #18
alexpottre #16 I think the code comment already covers this. This issue is a prioritized change (performance) as per https://www.drupal.org/core/beta-changes and it's benefits outweigh any disruption. Committed 43e265b and pushed to 8.0.x. Thanks!
Seems like we should have a tag and/or interface here. So any service can opt in to garbage collection on cron. But that is the stuff of followups.