From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <kostja@tarantool.org>
Date: Tue, 22 May 2018 16:56:34 +0300
From: Konstantin Osipov <kostja@tarantool.org>
Subject: Re: [PATCH 7/8] memtx: rework background garbage collection procedure
Message-ID: <20180522135634.GG11201@atlas>
References: <cover.1526987033.git.vdavydov.dev@gmail.com>
 <fca9ed8296d3f49b8ea253df2a4d0891858ab8e7.1526987033.git.vdavydov.dev@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <fca9ed8296d3f49b8ea253df2a4d0891858ab8e7.1526987033.git.vdavydov.dev@gmail.com>
To: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: tarantool-patches@freelists.org
List-ID: <tarantool-patches.dev.tarantool.org>

* Vladimir Davydov <vdavydov.dev@gmail.com> [18/05/22 15:10]:
> +/**
> + * 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;
> +}

Please rework this to avoid adding back tasks to the queue.

Peek the task in the queue and pop it if it's complete.

Using boolean for task completion status makes the code hard to
follow. I would use an out parameter.

>  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)) {

This is also counter to our typical use of return values.


Otherwise OK to push.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov