| Message ID | 20191215011649.12202-1-cus@passwd.hu |
|---|---|
| State | New |
| Headers | show |
On 15-12-2019 06:46 am, Marton Balint wrote: > Unfortunately the ld() and st() operations store the variables in the AVExpr > context and not in the parser in order to keep the stored values between > evaluations (which is a rarely(?) used undocumented(?) feature of AVExpr). > > This causes data race issues when the same expression is evaluated from > multiple threads. Let's use the Parser as variable store during evaluation and > only sync the AVExpr context variables with the Parser before and after > evaluation. This keeps the feature working for single threaded cases and fixes > using ld() and st() usage for the "normal" use case when the ld()-s only > reference st()-d values from the same evaluation. > > Maybe we should just remove this feature instead? No. I use them in many workflows, including select filter, and others involving animation. The eval API has a function to check if a variable is referenced in an expression. Michael has submitted a patch that extends that to allow checking for the use of any function. Once merged, filters can check if such st/ld is present in any expression and force thread count to 1, if need be. Gyan
On Sun, Dec 15, 2019 at 02:16:49AM +0100, Marton Balint wrote: > Unfortunately the ld() and st() operations store the variables in the AVExpr > context and not in the parser in order to keep the stored values between > evaluations (which is a rarely(?) used undocumented(?) feature of AVExpr). > > This causes data race issues when the same expression is evaluated from > multiple threads. Let's use the Parser as variable store during evaluation and > only sync the AVExpr context variables with the Parser before and after > evaluation. This keeps the feature working for single threaded cases and fixes > using ld() and st() usage for the "normal" use case when the ld()-s only > reference st()-d values from the same evaluation. > > Maybe we should just remove this feature instead? having some variable that is not local to the current point evaluation is needed for random() as it uses it as state ld/st are also documented in doc/utils.texi and are the basis for multiple features so they themselfs cant be removed either > > Fixes ticket #7528. > > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > libavutil/eval.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/libavutil/eval.c b/libavutil/eval.c > index 62d2ae938b..e726c94195 100644 > --- a/libavutil/eval.c > +++ b/libavutil/eval.c > @@ -54,7 +54,7 @@ typedef struct Parser { > int log_offset; > void *log_ctx; > #define VARS 10 > - double *var; > + double var[VARS]; > } Parser; > > static const AVClass eval_class = { > @@ -754,11 +754,14 @@ int av_expr_count_vars(AVExpr *e, unsigned *counter, int size) > double av_expr_eval(AVExpr *e, const double *const_values, void *opaque) > { > Parser p = { 0 }; > - p.var= e->var; > + double ret; > > + memcpy(p.var, e->var, sizeof(p.var)); > p.const_values = const_values; > p.opaque = opaque; > - return eval_expr(&p, e); > + ret = eval_expr(&p, e); > + memcpy(e->var, p.var, sizeof(p.var)); this does not look thread safe to me also it slows the code down by extra memcopies I think the main question is, do we need to share a expression evaluator between threads ? Is that useful to any feature or usecase ? If it has no use case then the obvious solution is not to share the expression evaluator or at least not the things written to per evaluation Maybe for combining statistics of pixels between threads but iam not sure how to do that with a shared expression evaluator efficeiently Anyone has any ideas what a shared expression evaluator could be usefull for ? Thanks [...]
On Sun, 15 Dec 2019, Michael Niedermayer wrote: > On Sun, Dec 15, 2019 at 02:16:49AM +0100, Marton Balint wrote: >> Unfortunately the ld() and st() operations store the variables in the AVExpr >> context and not in the parser in order to keep the stored values between >> evaluations (which is a rarely(?) used undocumented(?) feature of AVExpr). >> >> This causes data race issues when the same expression is evaluated from >> multiple threads. Let's use the Parser as variable store during evaluation and >> only sync the AVExpr context variables with the Parser before and after >> evaluation. This keeps the feature working for single threaded cases and fixes >> using ld() and st() usage for the "normal" use case when the ld()-s only >> reference st()-d values from the same evaluation. >> > >> Maybe we should just remove this feature instead? > > having some variable that is not local to the current point evaluation is > needed for random() as it uses it as state > ld/st are also documented in doc/utils.texi and are the basis for multiple > features so they themselfs cant be removed either lt() and st() is documented, but it is not documented that they keep their values between evaluations. >> { >> Parser p = { 0 }; >> - p.var= e->var; >> + double ret; >> >> + memcpy(p.var, e->var, sizeof(p.var)); >> p.const_values = const_values; >> p.opaque = opaque; >> - return eval_expr(&p, e); >> + ret = eval_expr(&p, e); >> + memcpy(e->var, p.var, sizeof(p.var)); > > this does not look thread safe to me It is thread safe if the expression is deterministic in a sense that it does not use the state variables without setting them first during a single evaluation. > also it slows the code down by extra memcopies > > I think the main question is, do we need to share a expression evaluator > between threads ? > Is that useful to any feature or usecase ? > If it has no use case then the obvious solution is not to share the expression > evaluator or at least not the things written to per evaluation The ticket this patch fix is a pretty good example where it matters. The confusion is caused by mixing the state with the parsed expression. Maybe we should decouple them, or at least add the possibility to the user to use a custom state. Anyway, I am inclined to accept what Gyan proposed, to determine if the expression uses lt() st() or rand() and if it does fall back to 1 thread. Or implement something like av_expr_is_stateless() to do this in a helper function. Regards, Marton
On Sun, Dec 15, 2019 at 01:52:52PM +0100, Marton Balint wrote: > > > On Sun, 15 Dec 2019, Michael Niedermayer wrote: > > >On Sun, Dec 15, 2019 at 02:16:49AM +0100, Marton Balint wrote: > >>Unfortunately the ld() and st() operations store the variables in the AVExpr > >>context and not in the parser in order to keep the stored values between > >>evaluations (which is a rarely(?) used undocumented(?) feature of AVExpr). > >> > >>This causes data race issues when the same expression is evaluated from > >>multiple threads. Let's use the Parser as variable store during evaluation and > >>only sync the AVExpr context variables with the Parser before and after > >>evaluation. This keeps the feature working for single threaded cases and fixes > >>using ld() and st() usage for the "normal" use case when the ld()-s only > >>reference st()-d values from the same evaluation. > >> > > > >>Maybe we should just remove this feature instead? > > > >having some variable that is not local to the current point evaluation is > >needed for random() as it uses it as state > >ld/st are also documented in doc/utils.texi and are the basis for multiple > >features so they themselfs cant be removed either > > lt() and st() is documented, but it is not documented that they keep their > values between evaluations. > > >> { > >> Parser p = { 0 }; > >>- p.var= e->var; > >>+ double ret; > >> > >>+ memcpy(p.var, e->var, sizeof(p.var)); > >> p.const_values = const_values; > >> p.opaque = opaque; > >>- return eval_expr(&p, e); > >>+ ret = eval_expr(&p, e); > >>+ memcpy(e->var, p.var, sizeof(p.var)); > > > >this does not look thread safe to me > > It is thread safe if the expression is deterministic in a sense that it does > not use the state variables without setting them first during a single > evaluation. I do not think writing different values from 2 threads without synchronization or any atomicity onto the same array of doubles and afterwards using this is safe. Even if we ignore C, you do not know if the frankenstein double value this could produce will function the same way a normal value would. Strictly it could crash on read i think > > >also it slows the code down by extra memcopies > > > >I think the main question is, do we need to share a expression evaluator > >between threads ? > >Is that useful to any feature or usecase ? > >If it has no use case then the obvious solution is not to share the expression > >evaluator or at least not the things written to per evaluation > > The ticket this patch fix is a pretty good example where it matters. i think you misunderstand, the example does not require inter thread communication and should be fine with a AVExpr per thread unless iam missing something. > > The confusion is caused by mixing the state with the parsed expression. > Maybe we should decouple them, or at least add the possibility to the user > to use a custom state. yes decoupling them / giving the user more control over the state makes sense > > Anyway, I am inclined to accept what Gyan proposed, to determine if the > expression uses lt() st() or rand() and if it does fall back to 1 thread. Or > implement something like av_expr_is_stateless() to do this in a helper > function. This solution kind of sucks because it forces a heap of cases to run single threaded which would work find with multiple threads OTOH with one AVExpr or AVExprState per thread it would work multi threaded The one case the state per thread will need to take a bit of care of is the random seed, which we would want to be different per thread Thanks [...]
On Sun, 15 Dec 2019, Michael Niedermayer wrote: > On Sun, Dec 15, 2019 at 01:52:52PM +0100, Marton Balint wrote: >> >> >> On Sun, 15 Dec 2019, Michael Niedermayer wrote: >> >>> On Sun, Dec 15, 2019 at 02:16:49AM +0100, Marton Balint wrote: >>>> Unfortunately the ld() and st() operations store the variables in the AVExpr >>>> context and not in the parser in order to keep the stored values between >>>> evaluations (which is a rarely(?) used undocumented(?) feature of AVExpr). >>>> >>>> This causes data race issues when the same expression is evaluated from >>>> multiple threads. Let's use the Parser as variable store during evaluation and >>>> only sync the AVExpr context variables with the Parser before and after >>>> evaluation. This keeps the feature working for single threaded cases and fixes >>>> using ld() and st() usage for the "normal" use case when the ld()-s only >>>> reference st()-d values from the same evaluation. >>>> >>> [...] >> >> The confusion is caused by mixing the state with the parsed expression. >> Maybe we should decouple them, or at least add the possibility to the user >> to use a custom state. > > yes decoupling them / giving the user more control over the state makes sense > > >> >> Anyway, I am inclined to accept what Gyan proposed, to determine if the >> expression uses lt() st() or rand() and if it does fall back to 1 thread. Or >> implement something like av_expr_is_stateless() to do this in a helper >> function. > > This solution kind of sucks because it forces a heap of cases to run > single threaded which would work find with multiple threads > OTOH with one AVExpr or AVExprState per thread it would work multi threaded That would resolve the data race issue in the ticket. However, if the expression depends on previous state then the result will be undeterministic because it is random which slices get assigned to which threads, so the initial AVExprState they see will be random as well. If you force single thread, the result will be deterministic. > > The one case the state per thread will need to take a bit of care of is the > random seed, which we would want to be different per thread We can just say that it is up to the user to initialize the random seed if needed, that is the case for single thread as well. Regards, Marton
On Sun, Dec 15, 2019 at 04:24:11PM +0100, Marton Balint wrote: > > > On Sun, 15 Dec 2019, Michael Niedermayer wrote: > > >On Sun, Dec 15, 2019 at 01:52:52PM +0100, Marton Balint wrote: > >> > >> > >>On Sun, 15 Dec 2019, Michael Niedermayer wrote: > >> > >>>On Sun, Dec 15, 2019 at 02:16:49AM +0100, Marton Balint wrote: > >>>>Unfortunately the ld() and st() operations store the variables in the AVExpr > >>>>context and not in the parser in order to keep the stored values between > >>>>evaluations (which is a rarely(?) used undocumented(?) feature of AVExpr). > >>>> > >>>>This causes data race issues when the same expression is evaluated from > >>>>multiple threads. Let's use the Parser as variable store during evaluation and > >>>>only sync the AVExpr context variables with the Parser before and after > >>>>evaluation. This keeps the feature working for single threaded cases and fixes > >>>>using ld() and st() usage for the "normal" use case when the ld()-s only > >>>>reference st()-d values from the same evaluation. > >>>> > >>> > > [...] > > >> > >>The confusion is caused by mixing the state with the parsed expression. > >>Maybe we should decouple them, or at least add the possibility to the user > >>to use a custom state. > > > >yes decoupling them / giving the user more control over the state makes sense > > > > > >> > >>Anyway, I am inclined to accept what Gyan proposed, to determine if the > >>expression uses lt() st() or rand() and if it does fall back to 1 thread. Or > >>implement something like av_expr_is_stateless() to do this in a helper > >>function. > > > >This solution kind of sucks because it forces a heap of cases to run > >single threaded which would work find with multiple threads > >OTOH with one AVExpr or AVExprState per thread it would work multi threaded > > That would resolve the data race issue in the ticket. However, if the > expression depends on previous state then the result will be undeterministic > because it is random which slices get assigned to which threads, so the > initial AVExprState they see will be random as well. well, something needs to be done to make it deterministic at least we can also make the ld/st pixel local and later add ways to access the left and top pixels state which could both be done with threads Either way question is really what is most usefull for expressions > > If you force single thread, the result will be deterministic. > > > > >The one case the state per thread will need to take a bit of care of is the > >random seed, which we would want to be different per thread > > We can just say that it is up to the user to initialize the random seed if > needed, that is the case for single thread as well. the user of the API yes but it should not be the command line user as that would be messy to do with an expression thanks [...]
diff --git a/libavutil/eval.c b/libavutil/eval.c index 62d2ae938b..e726c94195 100644 --- a/libavutil/eval.c +++ b/libavutil/eval.c @@ -54,7 +54,7 @@ typedef struct Parser { int log_offset; void *log_ctx; #define VARS 10 - double *var; + double var[VARS]; } Parser; static const AVClass eval_class = { @@ -754,11 +754,14 @@ int av_expr_count_vars(AVExpr *e, unsigned *counter, int size) double av_expr_eval(AVExpr *e, const double *const_values, void *opaque) { Parser p = { 0 }; - p.var= e->var; + double ret; + memcpy(p.var, e->var, sizeof(p.var)); p.const_values = const_values; p.opaque = opaque; - return eval_expr(&p, e); + ret = eval_expr(&p, e); + memcpy(e->var, p.var, sizeof(p.var)); + return ret; } int av_expr_parse_and_eval(double *d, const char *s,
Unfortunately the ld() and st() operations store the variables in the AVExpr context and not in the parser in order to keep the stored values between evaluations (which is a rarely(?) used undocumented(?) feature of AVExpr). This causes data race issues when the same expression is evaluated from multiple threads. Let's use the Parser as variable store during evaluation and only sync the AVExpr context variables with the Parser before and after evaluation. This keeps the feature working for single threaded cases and fixes using ld() and st() usage for the "normal" use case when the ld()-s only reference st()-d values from the same evaluation. Maybe we should just remove this feature instead? Fixes ticket #7528. Signed-off-by: Marton Balint <cus@passwd.hu> --- libavutil/eval.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)