RFC: Js of ocaml

[talex5]

On Firefox (with js_of_ocaml), I sometimes get "Stack overflow" errors parsing sexprs. It looks like js_of_ocaml can't see that the parser is tail recursive. This patch fixes it for me by inlining the helpers for processing string and atom characters. Ideally, it should be fixed properly for all cases, but in my application the rest of the structure is a fixed length so it doesn't affect me.

The bug doesn't always appear, even on the same input, so might depend on exactly what the JIT is doing. However, this reliably fails for me without the patch:

open Sexplib.Sexp

let () =
  let item =
"\"x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x\""
  in
  let parsed : t =
match parse item with
| Done (parsed, _pos) -> parsed
| Cont _ -> assert false in
  ignore parsed;
  print_endline "success"

If it passes for you, a longer string might trigger it better.

[ghost]

Sorry for the delay, I submitted the patch for review. We will need to revisit this code, especially I'm not sure the use of cpp is still required.

I'm not going to add the opam file for now as we don't have them in other janestreet repositories. If this is needed we can add them everywhere and add something to our release process to keep them up-to-date.

[talex5]

Thanks. The opam file is just so that people can pin this branch easily, but it is helpful to have them in general.

[talex5]

Any news on this? Would be nice to remove the opam pin.

[bmillwood]

This will be included in the next release, but the next release is taking a while because a bunch of code was moved around internally. I'm hopeful it won't be more than a week.

[talex5]

Cool - thanks!

[bmillwood]

I think 112.35.00 has the changes you need.