Gen.jl icon indicating copy to clipboard operation
Gen.jl copied to clipboard

Asymptotically inefficient evaluation of `@dist DSL` functions?

Open georgematheos opened this issue 5 years ago • 2 comments

This recursive function in the @dist DSL file worries me:

eval_arg(x::TransformedArg, args) =
    x.arg_passer(x.orig_f, [eval_arg(a, args) for a in x.f_args]...)

This is a classic "implement fib with vs without memoization" situation. We are not re-using calls to eval_arg for sub-arguments, we may evaluate the same eval_arg call in many subtrees; this could lead to poor asymptotic runtimes.

I'm not sure how major an issue this is, given that most of the uses of the @dist DSL have used pretty small function bodies. I also may be misunderstanding the code.

georgematheos avatar Oct 27 '20 21:10 georgematheos

For some @dist functions, which make assignments (like x = a + b) then use the results multiple times (y = x + x), caching would be more efficient, and an implementation that caches argument evaluations would be a welcome PR. I wasn't too worried about this, because the original goal of @dist was mostly to support one-liners that performed simple transformations of primitive distributions; I don't think I've seen @dist code yet that reuses transformed arguments in multiple places (which is not to say that it's not a reasonable thing to do in some cases!).

alex-lew avatar Oct 27 '20 22:10 alex-lew

That makes sense; I agree this does not need to be a priority!

At some point when we are discussing plans for Gen development, it would be good to discuss a way to organize bug tracking a bit better, to make it easier to categorize “bugs” like this which are essentially notes of potential issues that could arise in use cases we don’t expect to occur much, and which it is low-priority to fix. (Probably something like being more active with the github issue tags would suffice.)

georgematheos avatar Oct 28 '20 01:10 georgematheos