[PATCH 7/8] memtx: rework background garbage collection procedure

[Vladimir Davydov <vdavydov.dev@gmail.com>]

Currently, the engine has not control over yields issued during
asynchronous index destruction. As a result, it can't force gc when
there's not enough memory. To fix that, let's make gc callback stateful:
now it's supposed to free some objects and return true if there's still
more objects to free or false otherwise. Yields are now done by the
memtx engine itself after each gc callback invocation.
---
 src/box/index.cc       |  7 ++++++-
 src/box/memtx_engine.c | 33 ++++++++++++++++++++++++++++-----
 src/box/memtx_engine.h |  9 ++++++++-
 src/box/memtx_hash.c   | 23 +++++++++++------------
 src/box/memtx_hash.h   |  1 +
 src/box/memtx_tree.c   | 26 +++++++++++++-------------
 src/box/memtx_tree.h   |  1 +
 7 files changed, 68 insertions(+), 32 deletions(-)

diff --git a/src/box/index.cc b/src/box/index.cc
index 978935b3..b67f8bc6 100644
--- a/src/box/index.cc
+++ b/src/box/index.cc
@@ -496,8 +496,13 @@ index_create(struct index *index, struct engine *engine,
 void
 index_delete(struct index *index)
 {
-	index_def_delete(index->def);
+	/*
+	 * Free index_def after destroying the index as
+	 * engine might still need it.
+	 */
+	struct index_def *def = index->def;
 	index->vtab->destroy(index);
+	index_def_delete(def);
 }
 
 int
diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
index 51a47eff..3b21bcaa 100644
--- a/src/box/memtx_engine.c
+++ b/src/box/memtx_engine.c
@@ -933,19 +933,42 @@ static const struct engine_vtab memtx_engine_vtab = {
 	/* .check_space_def = */ memtx_engine_check_space_def,
 };
 
+/**
+ * Run one iteration of garbage collection. Return true if
+ * there still may be objects to collect, false otherwise.
+ */
+static bool
+memtx_engine_run_gc(struct memtx_engine *memtx)
+{
+	if (stailq_empty(&memtx->gc_queue))
+		return false;
+
+	struct memtx_gc_task *task = stailq_shift_entry(&memtx->gc_queue,
+					struct memtx_gc_task, link);
+	if (task->func(task)) {
+		/*
+		 * The task still has objects to collect,
+		 * put it back.
+		 */
+		stailq_add_entry(&memtx->gc_queue, task, link);
+	}
+	return true;
+}
+
 static int
 memtx_engine_gc_f(va_list va)
 {
 	struct memtx_engine *memtx = va_arg(va, struct memtx_engine *);
 	while (!fiber_is_cancelled()) {
-		if (stailq_empty(&memtx->gc_queue)) {
+		if (!memtx_engine_run_gc(memtx)) {
 			fiber_yield_timeout(TIMEOUT_INFINITY);
 			continue;
 		}
-		struct memtx_gc_task *task;
-		task = stailq_shift_entry(&memtx->gc_queue,
-					  struct memtx_gc_task, link);
-		task->func(task);
+		/*
+		 * Yield after each iteration so as not to block
+		 * tx thread for too long.
+		 */
+		fiber_sleep(0);
 	}
 	return 0;
 }
diff --git a/src/box/memtx_engine.h b/src/box/memtx_engine.h
index 0bcd24ac..934bc4e0 100644
--- a/src/box/memtx_engine.h
+++ b/src/box/memtx_engine.h
@@ -150,7 +150,14 @@ struct memtx_engine {
 };
 
 struct memtx_gc_task;
-typedef void (*memtx_gc_func)(struct memtx_gc_task *);
+
+/**
+ * Garbage collection callback. It is supposed to free some memory
+ * and return true if it still has more or false if it has freed
+ * everything it had. In the latter case, it must also free the
+ * task object.
+ */
+typedef bool (*memtx_gc_func)(struct memtx_gc_task *);
 
 /** Garbage collection task. */
 struct memtx_gc_task {
diff --git a/src/box/memtx_hash.c b/src/box/memtx_hash.c
index e1fdad54..67979b76 100644
--- a/src/box/memtx_hash.c
+++ b/src/box/memtx_hash.c
@@ -109,8 +109,8 @@ memtx_hash_index_free(struct memtx_hash_index *index)
 	free(index);
 }
 
-static void
-memtx_hash_index_destroy_f(struct memtx_gc_task *task)
+static bool
+memtx_hash_index_gc_f(struct memtx_gc_task *task)
 {
 	/*
 	 * Yield every 1K tuples to keep latency < 0.1 ms.
@@ -125,18 +125,17 @@ memtx_hash_index_destroy_f(struct memtx_gc_task *task)
 	struct memtx_hash_index *index = container_of(task,
 			struct memtx_hash_index, gc_task);
 	struct light_index_core *hash = &index->hash_table;
-
-	struct light_index_iterator itr;
-	light_index_iterator_begin(hash, &itr);
+	struct light_index_iterator *itr = &index->gc_iterator;
 
 	struct tuple **res;
 	unsigned int loops = 0;
-	while ((res = light_index_iterator_get_and_next(hash, &itr)) != NULL) {
+	while ((res = light_index_iterator_get_and_next(hash, itr)) != NULL) {
 		tuple_unref(*res);
-		if (++loops % YIELD_LOOPS == 0)
-			fiber_sleep(0);
+		if (++loops >= YIELD_LOOPS)
+			return true;
 	}
 	memtx_hash_index_free(index);
+	return false;
 }
 
 static void
@@ -144,12 +143,15 @@ memtx_hash_index_destroy(struct index *base)
 {
 	struct memtx_hash_index *index = (struct memtx_hash_index *)base;
 	struct memtx_engine *memtx = (struct memtx_engine *)base->engine;
-	if (index->gc_task.func != NULL) {
+	if (base->def->iid == 0) {
 		/*
 		 * Primary index. We need to free all tuples stored
 		 * in the index, which may take a while. Schedule a
 		 * background task in order not to block tx thread.
 		 */
+		index->gc_task.func = memtx_hash_index_gc_f;
+		light_index_iterator_begin(&index->hash_table,
+					   &index->gc_iterator);
 		memtx_engine_schedule_gc(memtx, &index->gc_task);
 	} else {
 		/*
@@ -456,9 +458,6 @@ memtx_hash_index_new(struct memtx_engine *memtx, struct index_def *def)
 	light_index_create(&index->hash_table, MEMTX_EXTENT_SIZE,
 			   memtx_index_extent_alloc, memtx_index_extent_free,
 			   memtx, index->base.def->key_def);
-
-	if (def->iid == 0)
-		index->gc_task.func = memtx_hash_index_destroy_f;
 	return index;
 }
 
diff --git a/src/box/memtx_hash.h b/src/box/memtx_hash.h
index 7a08dc87..a3b48052 100644
--- a/src/box/memtx_hash.h
+++ b/src/box/memtx_hash.h
@@ -72,6 +72,7 @@ struct memtx_hash_index {
 	struct index base;
 	struct light_index_core hash_table;
 	struct memtx_gc_task gc_task;
+	struct light_index_iterator gc_iterator;
 };
 
 struct memtx_hash_index *
diff --git a/src/box/memtx_tree.c b/src/box/memtx_tree.c
index 8814b13b..5a5d7fe1 100644
--- a/src/box/memtx_tree.c
+++ b/src/box/memtx_tree.c
@@ -308,8 +308,8 @@ memtx_tree_index_free(struct memtx_tree_index *index)
 	free(index);
 }
 
-static void
-memtx_tree_index_destroy_f(struct memtx_gc_task *task)
+static bool
+memtx_tree_index_gc_f(struct memtx_gc_task *task)
 {
 	/*
 	 * Yield every 1K tuples to keep latency < 0.1 ms.
@@ -320,21 +320,22 @@ memtx_tree_index_destroy_f(struct memtx_gc_task *task)
 #else
 	enum { YIELD_LOOPS = 10 };
 #endif
+
 	struct memtx_tree_index *index = container_of(task,
 			struct memtx_tree_index, gc_task);
 	struct memtx_tree *tree = &index->tree;
+	struct memtx_tree_iterator *itr = &index->gc_iterator;
 
 	unsigned int loops = 0;
-	struct memtx_tree_iterator itr;
-	for (itr = memtx_tree_iterator_first(tree);
-	     !memtx_tree_iterator_is_invalid(&itr);
-	     memtx_tree_iterator_next(tree, &itr)) {
-		struct tuple *tuple = *memtx_tree_iterator_get_elem(tree, &itr);
+	while (!memtx_tree_iterator_is_invalid(itr)) {
+		struct tuple *tuple = *memtx_tree_iterator_get_elem(tree, itr);
+		memtx_tree_iterator_next(tree, itr);
 		tuple_unref(tuple);
-		if (++loops % YIELD_LOOPS == 0)
-			fiber_sleep(0);
+		if (++loops >= YIELD_LOOPS)
+			return true;
 	}
 	memtx_tree_index_free(index);
+	return false;
 }
 
 static void
@@ -342,12 +343,14 @@ memtx_tree_index_destroy(struct index *base)
 {
 	struct memtx_tree_index *index = (struct memtx_tree_index *)base;
 	struct memtx_engine *memtx = (struct memtx_engine *)base->engine;
-	if (index->gc_task.func != NULL) {
+	if (base->def->iid == 0) {
 		/*
 		 * Primary index. We need to free all tuples stored
 		 * in the index, which may take a while. Schedule a
 		 * background task in order not to block tx thread.
 		 */
+		index->gc_task.func = memtx_tree_index_gc_f;
+		index->gc_iterator = memtx_tree_iterator_first(&index->tree);
 		memtx_engine_schedule_gc(memtx, &index->gc_task);
 	} else {
 		/*
@@ -690,8 +693,5 @@ memtx_tree_index_new(struct memtx_engine *memtx, struct index_def *def)
 	struct key_def *cmp_def = memtx_tree_index_cmp_def(index);
 	memtx_tree_create(&index->tree, cmp_def, memtx_index_extent_alloc,
 			  memtx_index_extent_free, memtx);
-
-	if (def->iid == 0)
-		index->gc_task.func = memtx_tree_index_destroy_f;
 	return index;
 }
diff --git a/src/box/memtx_tree.h b/src/box/memtx_tree.h
index 2e7acde2..bbc8b241 100644
--- a/src/box/memtx_tree.h
+++ b/src/box/memtx_tree.h
@@ -98,6 +98,7 @@ struct memtx_tree_index {
 	struct tuple **build_array;
 	size_t build_array_size, build_array_alloc_size;
 	struct memtx_gc_task gc_task;
+	struct memtx_tree_iterator gc_iterator;
 };
 
 struct memtx_tree_index *
-- 
2.11.0