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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

moshe weitzman’s picture

I like moving garbage collection into cron.

sergeypavlenko’s picture

Hello. I support the idea, move cleaning tables in cron. Cleaning tables at each page load may affect performance.

andypost’s picture

This destruction better to replace with invalidation before usage

andypost’s picture

GC could work as queueItem about collection

Spleshka’s picture

Also support idea to use queue API, so on cron gc can drop expired data.

Berdir’s picture

Status: Active » Needs review
FileSize
5.98 KB

I'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.

Status: Needs review » Needs work

The last submitted patch, 6: cleanup-keyvalue-on-cron-2186113-5.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
8.21 KB
2.87 KB

Fixing the tests.

andypost’s picture

That'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 issue

Status: Needs review » Needs work

The last submitted patch, 8: cleanup-keyvalue-on-cron-2186113-8.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
7.52 KB

How about that RTBC? ;)

Wim Leers’s picture

Status: Needs review » Needs work

Looks great! Only silly nitpicks:

  1. +++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueDatabaseExpirableFactory.php
    @@ -61,14 +61,11 @@ public function get($collection) {
    +  public function garbageCollection() {
    

    s/garbageCollection()/garbageCollect()/

    i.e. verb, not noun?

  2. +++ b/core/modules/system/src/Tests/KeyValueStore/GarbageCollectionTest.php
    @@ -28,9 +28,10 @@ class GarbageCollectionTest extends KernelTestBase {
    +    // The additional tables are necessary due to the call to system_cron().
    

    Either s/The/These/ or s/are//.

  3. +++ b/core/modules/system/system.module
    @@ -1154,6 +1153,11 @@ function system_cron() {
    +  // Cleanup the expirable key value database store.
    

    s/Cleanup/Clean up/

Berdir’s picture

Status: Needs work » Needs review
FileSize
8.02 KB
1.87 KB

1. 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 :)

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Hah! :)

catch’s picture

+++ b/core/modules/system/src/Tests/KeyValueStore/GarbageCollectionTest.php
@@ -58,10 +59,10 @@ public function testGarbageCollection() {
+    system_cron();

Very 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.

Berdir’s picture

I 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...

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

re #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!

+++ b/core/modules/system/system.module
@@ -1144,17 +1143,22 @@ function system_get_module_admin_tasks($module, array $info) {
-  // Cleanup the flood.
+  // Clean up the flood.
   \Drupal::flood()->garbageCollection();
 
   foreach (Cache::getBins() as $cache_backend) {
     $cache_backend->garbageCollection();
   }
 
-  // Cleanup the queue for failed batches.
+  // Clean up the expirable key value database store.
+  if (\Drupal::service('keyvalue.expirable.database') instanceof KeyValueDatabaseExpirableFactory) {
+    \Drupal::service('keyvalue.expirable.database')->garbageCollection();
+  }

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.

  • alexpott committed 43e265b on 8.0.x
    Issue #2186113 by Berdir: Avoid key value expire garbage collection on...

Status: Fixed » Closed (fixed)

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