Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Js of ocaml #14

Closed
wants to merge 2 commits into from
Closed

Conversation

@talex5
Copy link

@talex5 talex5 commented Apr 26, 2015

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.

talex5 added 2 commits Apr 14, 2015
Other places are still not tail-recursive, but this is the main problem.
@jeremiedimino
Copy link
Member

@jeremiedimino jeremiedimino commented May 14, 2015

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
Copy link
Author

@talex5 talex5 commented May 14, 2015

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

@talex5
Copy link
Author

@talex5 talex5 commented Jun 13, 2015

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

@bmillwood
Copy link
Contributor

@bmillwood bmillwood commented Jun 15, 2015

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
Copy link
Author

@talex5 talex5 commented Jun 15, 2015

Cool - thanks!

bmillwood added a commit that referenced this pull request Jun 19, 2015
- Inline some calls that js_of_ocaml was unable to recognise as
  tail-recursive (cf. issue #14)
@bmillwood
Copy link
Contributor

@bmillwood bmillwood commented Jul 14, 2015

I think 112.35.00 has the changes you need.

@bmillwood bmillwood closed this Jul 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants