[vlc-devel] [PATCH 2/6] executor: add unit tests
Romain Vimont
rom1v at videolabs.io
Tue Sep 8 10:01:25 CEST 2020
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.
Yes, there are two alternatives:
1. either the executor allocates its own runnable
2. either the caller is responsible to provide the runnable instance
(1.) > (2.):
- vlc_executor_Submit() returns void (it simplifies error handling on
the caller-side)
- a single allocation for the client task and the executor runnable
- runnables can be on the stack
- no need for a runnable destructor in the API (?)
(1.) < (2.):
- if the caller wants to submit the same "task" several times, it must
provide a new runnable every time (it will often have to allocate a
new task anyway, but here it does not)
- the executor-private part of the runnable structure is exposed
- if we need more executor-private data than a single list node, we
don't want to add new fields (cf [1])
[1]: https://mailman.videolan.org/pipermail/vlc-devel/2020-September/137203.html
"runnable->node.prev = runnable->node.next = NULL;"
In both alternatives, the caller owns the runnable (it is responsible to
delete it).
I think I'm ok with both.
> > + 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.
Sorry, I don't understand your point (what does it change that they are
on the stack or heap-allocated?)
> > +
> > + 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)
On the other hand, changing the field order will break all call sites
(possibly silently if the types are compatible).
And here, the struct contains is a third field ("node"), which we don't
want to explicitly initialize.
>
> > +
> > + 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.
>
> > +
> > + 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 */
> > +
> > + 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);
> > + 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