[vlc-devel] [RFC v2 1/2] executor: introduce new executor API

[Rémi Denis-Courmont]

Le keskiviikkona 2. syyskuuta 2020, 9.13.15 EEST Steve Lhomme a écrit :
> > +     *
> > +     * \param userdata the userdata provided to vlc_executor_Submit()
> > +     */
> > +    void (*run)(void *userdata);
> 
> Since the userdata is also part of the vlc_runnable, you may pass the
> vlc_runnable instead of the userdata. It allows more flexibilty in the
> future (for example if you want to do actions on the runnable, like run
> it again).

There are at least 4 different ways to paint that shed...

1) Pass only the runnable, and remove the user data. The callback uses 
container_of() to get back to its private data.

2) Pass the runnable, and leave the user data inside for the callback to 
fetch.

3) Pass both the runnable and the user data. The callback will likely need to 
cast to void due to unused argument warnings.

4) Pass only the user data, as here. The callback can get back to the runnabel 
from within its private data if needed.

I'd argue that (4) (this patch) is the cleanest. I don't think it matters 
much, as long as it's consistent though.

(...)
> And in fact you don't need any userdata at all. You just wrap your
> runnable in a structure with the data you want. (as an object that would
> support the runnable interface)

Yes, I already said in the previous version's thread that you could optionally 
do optionally away with the user data.

That's the kind of memory usage micro-optimization that makes most sense in 
kernel space or embedded code though.

(...)

> > +void
> > +vlc_executor_Submit(vlc_executor_t *executor, struct vlc_runnable
> > *runnable) +{
> > +    vlc_mutex_lock(&executor->lock);
> > +
> > +    assert(!executor->closing);
> 
> It might be legitimate to try to submit a task even if the executor is
> closing. For example if a runnable is trying to submit another runnable.
> It should have to check with some higher power if it's allowed to do so
> or not (atomically). So at least you should just return here.

Insofar as we cannot return an error, we have to run the job in that case.

And as I said earlier, I really don't think we should return an error, because 
it's very likely that the caller cannot handle it.


I don't have a strong opinion on allowing nested queueing. But if we do allow, 
I think having a proper unit test is a MUST.

-- 
Реми Дёни-Курмон
http://www.remlab.net/