[vlc-devel] [PATCH 2/6] executor: add unit tests
Alexandre Janniaux
ajanni at videolabs.io
Tue Sep 8 10:10:58 CEST 2020
Hi,
Well done for this test :)
On Tue, Sep 08, 2020 at 09:39:10AM +0200, Steve Lhomme wrote:
> On 2020-09-07 17:40, Romain Vimont wrote:
> > ...
> > +static void test_multiple_runnables(void)
> > +{
> > + vlc_executor_t *executor = vlc_executor_New(3);
> > + assert(executor);
> > +
> > + struct data shared_data;
> > + InitData(&shared_data);
> > +
> > + struct vlc_runnable runnables[300];
>
> This is very inconvenient that you have to allocate and fill 300 times the
> same structure exactly the same way. A single instance that you submit 300
> would be better.
Since each task is tracked by the corresponding runnable
I don't think it's possible, but maybe the current syntax
is a recipee for failure without stronger assertion that
you don't submit an already submitted task?
However, why 300 and not 10? Or 2? I guess the goal is to
run multiple jobs while having more jobs than the number of
threads so you could add a comment to rationalize the choice
a little.
> > + for (int i = 0; i < 300; ++i)
> > + {
> > + struct vlc_runnable *runnable = &runnables[i];
> > + runnable->run = RunIncrement;
> > + runnable->userdata = &shared_data;
> > + vlc_executor_Submit(executor, runnable);
> > + }
> > +
> > + vlc_mutex_lock(&shared_data.lock);
> > + while (shared_data.ended < 300)
> > + vlc_cond_wait(&shared_data.cond, &shared_data.lock);
> > + vlc_mutex_unlock(&shared_data.lock);
>
> Since the runnables are in the stack, they could keep track whether they are
> finished running or not, and be signaled by the executor when they are. But
> I understand that means adding more things to the runnable even when the
> feature is not used.
In particular, it means adding new synchronization while
it's not needed if you chain tasks.
> > +
> > + assert(shared_data.ended == 300);
> > +
> > + vlc_executor_Delete(executor);
> > +}
> > +
> > +static void test_blocking_delete(void)
> > +{
> > + vlc_executor_t *executor = vlc_executor_New(1);
> > + assert(executor);
> > +
> > + struct data data;
> > + InitData(&data);
> > +
> > + data.delay = VLC_TICK_FROM_MS(100);
> > +
> > + struct vlc_runnable runnable = {
> > + .run = RunIncrement,
> > + .userdata = &data,
> > + };
>
> Usually I prefer to initialize without the field names. That makes
> refactoring safer as you can:
> - change the field name without changing all user code
> - add callbacks and be warned (or fail to compile) if a user is missing an
> implementation (or NULL)
I prefer to initialize with a designated initializer, it
zero-initialize the other fields when needed. and especially
here where you don't want to specify the intializer for the
vlc_list. :)
> > +
> > + vlc_executor_Submit(executor, &runnable);
> > +
> > + /* Wait for the runnable to be started */
> > + vlc_mutex_lock(&data.lock);
> > + while (data.started == 0)
> > + vlc_cond_wait(&data.cond, &data.lock);
> > + vlc_mutex_unlock(&data.lock);
> > +
> > + /* The runnable sleeps for about 100ms */
>
> You may make it longer than 100ms as the test sometimes run in parallel with
> other things on busy systems.
If it could not take too much time, we'd probably appreciate
it on Linux where tests are taking some time already though.
I don't know if it's possible to test the correct interleave
of execution without risking a race too, and without sleep,
but that would be very appreciated.
> > +
> > + vlc_executor_Delete(executor);
> > +
> > + vlc_mutex_lock(&data.lock);
> > + /* The executor must wait the end of running tasks on delete */
> > + assert(data.ended == 1);
> > + vlc_mutex_unlock(&data.lock);
> > +}
> > +
> > +static void test_cancel(void)
> > +{
> > + vlc_executor_t *executor = vlc_executor_New(4);
> > + assert(executor);
> > +
> > + struct data shared_data;
> > + InitData(&shared_data);
> > +
> > + shared_data.delay = VLC_TICK_FROM_MS(100);
> > +
> > + /* Submit 40 tasks taking at least 100ms on an executor with at most 4
> > + * threads */
Why 40 too here? given that the total account for 1sec, it
seems a bit overkill.
> > +
> > + struct vlc_runnable runnables[40];
> > + for (int i = 0; i < 40; ++i)
> > + {
> > + struct vlc_runnable *runnable = &runnables[i];
> > + runnable->run = RunIncrement;
> > + runnable->userdata = &shared_data;
> > + vlc_executor_Submit(executor, runnable);
> > + }
> > +
> > + /* Wait a bit (in two lines to avoid harmful_delay() warning) */
> > + vlc_tick_t delay = VLC_TICK_FROM_MS(150);
> > + vlc_tick_sleep(delay);
> > +
> > + int canceled = 0;
> > + for (int i = 0; i < 40; ++i)
> > + {
> > + if (vlc_executor_Cancel(executor, &runnables[i]))
> > + ++canceled;
> > + }
> > +
> > + vlc_mutex_lock(&shared_data.lock);
> > + /* All started must not be canceled (but some non-started-yet may not be
> > + * canceled either) */
> > + assert(canceled + shared_data.started <= 40);
Shouldn't we test a == 40 then?
It seems to me that this assertion should be improved for this
and thus take into account the tasks that have started but not
incremented the counter yet. Otherwise you might have task
cancelled counting for two and task started counting for zero.
> > + vlc_mutex_unlock(&shared_data.lock);
> > +
> > + vlc_executor_Delete(executor);
> > +
> > + /* Every started task must also be ended */
> > + assert(shared_data.started == shared_data.ended);
> > + /* Every task is either canceled or ended */
> > + assert(canceled + shared_data.ended == 40);
> > +}
> > +
> > +struct doubler_task
> > +{
> > + vlc_executor_t *executor;
> > + int *array;
> > + size_t count;
> > + struct vlc_runnable runnable;
> > +};
> > +
> > +static void DoublerRun(void *);
> > +
> > +static bool
> > +SpawnDoublerTask(vlc_executor_t *executor, int *array, size_t count)
> > +{
> > + struct doubler_task *task = malloc(sizeof(*task));
> > + if (!task)
> > + return false;
> > +
> > + task->executor = executor;
> > + task->array = array;
> > + task->count = count;
> > + task->runnable.run = DoublerRun;
> > + task->runnable.userdata = task;
> > +
> > + vlc_executor_Submit(executor, &task->runnable);
> > +
> > + return true;
> > +}
> > +
> > +static void DoublerRun(void *userdata)
> > +{
> > + struct doubler_task *task = userdata;
> > +
> > + if (task->count == 1)
> > + task->array[0] *= 2; /* double the value */
> > + else
> > + {
> > + /* Spawn tasks doubling halves of the array recursively */
> > + bool ok;
> > +
> > + ok = SpawnDoublerTask(task->executor, task->array, task->count / 2);
> > + assert(ok);
> > +
> > + ok = SpawnDoublerTask(task->executor, task->array + task->count / 2,
> > + task->count - task->count / 2);
> > + assert(ok);
> > + }
> > +
> > + free(task);
> > +}
> > +
> > +static void test_task_chain(void)
> > +{
> > + vlc_executor_t *executor = vlc_executor_New(4);
> > + assert(executor);
> > +
> > + /* Numbers from 0 to 99 */
> > + int array[100];
> > + for (int i = 0; i < 100; ++i)
> > + array[i] = i;
> > +
> > + /* Double all values in the array from tasks spawning smaller tasks
> > + * recursively, until the array has size 1, where the single value is
> > + * doubled */
> > + SpawnDoublerTask(executor, array, 100);
> > +
> > + vlc_executor_WaitIdle(executor);
> > + vlc_executor_Delete(executor);
> > +
> > + /* All values must have been doubled */
> > + for (int i = 0; i < 100; ++i)
> > + assert(array[i] == 2 * i);
> > +}
> > +
> > +int main(void)
> > +{
> > + test_single_runnable();
> > + test_multiple_runnables();
> > + test_blocking_delete();
> > + test_cancel();
> > + test_task_chain();
> > + return 0;
> > +}
> > --
> > 2.28.0
> >
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> >
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list