[vlc-devel] [PATCH 2/6] executor: add unit tests

[Romain Vimont]

On Tue, Sep 08, 2020 at 10:10:58AM +0200, Alexandre Janniaux wrote:
> 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?

Yes, I agree, because when I wrote the test, I was tempted to use the
same runnable, since I initialize all of them exactly the same way (only
the "node" is different, but we don't see it).

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

I had no idea which numbers to choose. I just picked random values. :D

Here, the tasks are almost immediate anyway, so 10 or 5000 does not
change anything for the test.

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

+1

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

Same as for 300, I just picked a value. The tasks are canceled after
150ms anyway.

> > > +
> > > +    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?

Not here, because even if vlc_executor_Cancel() avoids a race-condition
(if it return false, we know that run() will be called), the "started"
value is incremented in the run() implementation (and may be not
executed yet).

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

:) but you can do that only after the _Delete (or WaitIdle()).

> Otherwise you might have task
> cancelled counting for two and task started counting for zero.

(I think you may not have a task canceled counting for two though)

> 
> > > +    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);

Like this ^

> > > +}
> > > +
> > > +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