Open pbaudin opened 3 years ago
I believe that exception-raising is also fairly slow with js-of-ocaml, so I suspect that the version you propose could be a noticeable performance regression for small lists.
The performance regression mainly depends on the number of lists. Of course, if these lists are small it is more noticeable.
I looked at this implementation :
let rec map_bind f acc xs =
match xs with
| x :: xs -> (match f x with
| ((Result.Error _) as x) -> x
| Result.Ok x -> map_bind f (x :: acc) xs)
| [] -> Result.Ok (List.rev acc)
It seems to be ok with my example (no stack overflow), but I wasn't able to extract its javascript translation since the debugger shows me only my Ocaml source code ! So, I can't confirm that is the solution.
Yes; what you did is to inline the implementation of >>=
, and I believe that the result is using a form of tail-recursion that is properly supported by js_of_ocaml. Would you like to propose a PR with this new implementation?
Note: what we learned here is that common monadic-programming idioms (recursive functions with bindings inside) are not properly supported by js_of_ocaml. I wondered if this may be a problem as well for the code generated by ppx_deriving_yojson.
I inspected the uses of >>=
in the codebase, and it appears that they are all in fact use-cases in which map
and pair
would suffice (in other words, we are only using the applicative structure of the monad), because they are of the form <foo> >>= (fun x -> <bar> >>= fun y -> Result.Ok (... x ... y ... ))
. This means that there may be no problem in practice, and that it should be possible to rewrite the code generator to guarantee that there is no problem -- if instead we wrote the code above like map (pair <foo> <bar>) (fun (x, y) -> Result.Ok (... x ... y ...)
, we would know for sure that the recursive calls are never "hidden" inside a closure like they are today. Let me know if you would be interested in contributing this change, I'm happy to provide more explanations.
I moved my latest implementation in a part where I can see the javascript translation :
function safe_map_bind(f,acc,xs)
{var acc$0=acc,xs$0=xs;
for(;;)
{if(xs$0)
{var xs$1=xs$0[2],x=xs$0[1],x$0=caml_call1(f,x);
if(0 === x$0[0])
{var x$1=x$0[1],acc$1=[0,x$1,acc$0],acc$0=acc$1,xs$0=xs$1;continue}
return x$0}
return [0,caml_call1(Base_List[35],acc$0)]}}
That looks safe since there is no recursion into the generated code!
I do hope you will modify your Ppx_deriving_yojson_runtime.ml
in that way.
Thanks.
I do hope you will modify your Ppx_deriving_yojson_runtime.ml in that way.
I asked above if you would be willing to send a Pull Request to implement the change. (Is this an implicit way to decline the invitation?). I think that a PR is nicer than a maintainer doing the change, in particular as it directly tracks authorship of the contribution.
I would have greatly preferred to let the authorship of that inline to the compiler!
Within the JavaScript mode the compilation of the recursive functions is not optimized (c.f. http://ocsigen.org/js_of_ocaml/3.7.0/manual/tailcall).
There is the Javascript translation of your
Ppx_deriving_yojson_runtime.map_bind
function :So, the conversion of a
Yojson.Safe.t
data into a long list of elements (~10.000) could raise aStack overflow
exception.Notice that kind of implementation (without
acc
) solves the problem:Of course, the proposed implementation can certainly be improved since the type of that
save_map_bind
function is less generic than yourmap_bind
function (but it seems to me that the proposed implementation is compliant with the.mli
interface except about the removed accumulator).