Stream to Seq

[gasche]

Before:

┌─────────────────────────────────────┬──────────┬────────────┬────────────┬──────────┬────────────┐
│ Name                                │ Time/Run │    mWd/Run │   mjWd/Run │ Prom/Run │ Percentage │
├─────────────────────────────────────┼──────────┼────────────┼────────────┼──────────┼────────────┤
│ JSON stream roundtrip               │ 998.06us │ 70_051.93w │ 10_985.58w │   13.98w │    100.00% │
└─────────────────────────────────────┴──────────┴────────────┴────────────┴──────────┴────────────┘

After:

┌────────────────────┬──────────┬────────────┬────────────┬──────────┬────────────┐
│ Name               │ Time/Run │    mWd/Run │   mjWd/Run │ Prom/Run │ Percentage │
├────────────────────┼──────────┼────────────┼────────────┼──────────┼────────────┤
│ JSON seq roundtrip │ 907.16us │ 60_038.93w │ 10_980.64w │    8.75w │    100.00% │
└────────────────────┴──────────┴────────────┴────────────┴──────────┴────────────┘

[gasche]

See #129.

[Leonidas-from-XIV]

That's great.

The only problem I see is with stdcompat. After seing how it is built and maintained (autotools, no dune, which creates issues for us using it in the duniverse and is also kinda a large amount of code to depend on) I would much prefer a different solution. Given we infamously use CPPO we could have our own Seq module for older versions of OCaml or something.

[gasche]

Actually I think I could use c-cube/seq instead, I can update the PR, but only later in the day (or feel free to do it if you want).

[Leonidas-from-XIV]

That's a good point, @c-cube's seq is exactly the kind of library I hoped for.

[c-cube]

Suggested change

	- The stream_from_* and stream_to_* functions now use a Seq.t instead of a Stream.t,
	- The `stream_from_*` and `stream_to_*` functions now use a Seq.t instead of a Stream.t,

[Leonidas-from-XIV]

Done, also quoted the other identifiers.

[Leonidas-from-XIV]

Ok, reopening this since the force-push undid my change.

[c-cube]

Something I'm wondering about is whether it's worth it trying to make the seq persistent. It's not too hard to do, for a small cost compared to json decoding, and it can be convenient if one uses the Seq as a functional value. In pseudo code:

let rec stream_to_seq ic : t Seq.t =
  let r = ref None in
  fun () ->
    match !r with
    | Some node -> node
    | None ->
      let node =
        try
          let json = decode_json ic in
          Seq.Cons (j, stream_to_seq ic)
        with End_of_file -> Seq.Nil
      in
      r := Some node;
      node

[Leonidas-from-XIV]

Some things I have encountered while going through the code.

[gasche]

@c-cube I tried to implement your suggestion, it's a bit more work because we need to handle exceptions as well. I got the following version which is 2x slower and allocates 3x more.

  type 'a cache =
    | Empty
    | Full of 'a
    | Fail of exn

  let seq_from_lexbuf v ?(fin = fun () -> ()) lexbuf =
    let stream = Some true in
    let rec f lexbuf =
      let cache = ref Empty in
      fun () ->
        match !cache with
        | Full node -> node
        | Fail e -> raise e
        | Empty ->
          let node =
            try Seq.Cons (from_lexbuf v ?stream lexbuf, f lexbuf)
            with
            | End_of_input ->
              fin ();
              Seq.Nil
            | e ->
              let exn =
                try ignore (fin ()); e
                with fin_e -> Finally (e, fin_e)
              in
              cache := Fail exn;
              raise exn
          in
          cache := Full node;
          node
    in
    f lexbuf

I think it's simpler to stay with the non-persistent version, and let users convert it explicitly to a persistent version if they need to. (I expect that they rarely do.)

Note: (even before @c-cube chimed in,) I was wondering whether one of containers' generator/iterator types (those with type unit -> 'a option, basically) could be an even better fit than Seq.t, because (they're close to how the code is written and) they are more obviously non-persistent. But I think that reusing a type from the standard library has value.

[c-cube]

@gasche I'm surprised by the overhead, but so be it :-). I think it's indeed valuable to use Seq as it's the most common type now. Adding a gen function on the side is doable but probably won't benefit as many people.

[gasche]

(Note: I'm not working from a flambda switch.)

[c-cube]

(Note: I'm not working from a flambda switch.)

Most people aren't, so I think it's reasonable 😁

[tcoopman]

By coincidence I stumbled on this PR, I was looking into what streaming functions yojson had to offer and because this PR was just opened, I wanted to try it.

Now I have an issue with Yojson.Basic.seq_to_file "path/to/file" seq. It runs fine, creates the file, but the file stays empty?

If I run the exact same code but with Yojson.Basic.seq_to_string it outputs the correct string

Any idea if I'm doing something wrong, or if something else is going on?
I'll see if I can find a minimal reproduction

[tcoopman]

Here is a minimal (I'm using base) that fails:

let test () =
  let seq = Sequence.of_list [`String "a"; `String "b"; `String "c"] |> Sequence.to_seq in
  Yojson.Basic.seq_to_string seq |> Stdio.print_endline;
  Yojson.Basic.seq_to_file "test.json" seq

This prints "a""b""c" on terminal, but "test.json" is empty.
I'm on linux with ocaml 4.13.1

[gasche]

This does look like a bug indeed, that (according to my test) was already present in the Stream functions this PR is replacing. (I guess seq_to_file isn't used much!)

[tcoopman]

This line is missing a line to actually write the buffer Buffer.output_buffer oc ob

[gasche]

Yep, I just found the same fix. I think it should be fixed separately from this PR.

[Leonidas-from-XIV]

I think it is okay to add this to this PR, but it is up to you.

[tcoopman]

I think the fix should be the suggestion made by @gasche in the comments, otherwise the seq has no real value as there is no memory improvement.

[gasche]

I'm planning to send a separate PR with the fix, and probably the streaming improvement as well, but probably not before tomorrow. I think the current PR should stay focused on just replacing Streaming with Seq, preserving the behavior exactly.

There are other things that look fishy in the buffer-handling logic of write.mll (I suspect that it could sometimes include the existing data of the buffer in the output, which is not intended), so it would be nice to review that as well, maybe I'll do it.

[tcoopman]

@gasche just found it - see my review - it's missing a line to write the buffer. At least that is how the regular to_channel works

[tcoopman]

I've noticed something else that's strange (so after I fixed writing the buffer as well). I'm not sure if it's an issue with this PR or something I'm not understanding correctly.

When I'm doing seq_to_file my memory usage of the program grows to 14GB (I'm generating a file of over 5GB.
But when I just use Seq.iter (fun _ -> ()) seq the memory usage never goes over 40MB. The execution time of the program is similar (1m9s for writing vs 52s without writing) so I'm pretty sure I'm doing the same work

Isn't the goal of the seq_to_file that you don't have to keep the whole file in memory?

(if you prefer this question somewhere else or as bug reports, let me know)

[gasche]

Could you try the following implementation?

let seq_to_channel ?buf ?(len=2096) ?std oc st =
  let ob =
    match buf with
        None -> Buffer.create len
      | Some ob -> ob
  in
  Seq.iter (fun v ->
    to_buffer ?std ob v;
    Buffer.output_buffer oc ob;
    Buffer.clear ob;
  ) st

[tcoopman]

Awesome, that fixes it! ❤️

[gasche]

The CI failed on <4.07 versions, I think it's because I forgot to add the seq dependency to dune in addition to yojson.opam, I just pushed an updated version.

[gasche]

See #133 (fixing the bugs reported by @tcoopman) and #132 (improving the usage of the ?buf parameter in other write functions). #133 conflicts with the current PR, I'll let maintainers decide which one they want to merge (if any) and rebase the other accordingly.

[Leonidas-from-XIV]

I would suggest going with #133 first. I took a look at it and it is ready to be merged in my opinion (despite the failure on 4.14 in CI, but this is gonna be handled in the PR) then rebase this PR on it and then tackle #132 after this is merged. Thanks for your effort @gasche and @tcoopman, much appreciated! There's apparently big parts of Yojson that are under-utilized/under-tested so it is great that these things are getting reported and fixed.

[Leonidas-from-XIV]

@gasche #133 is merged, can you rebase please? That would fix our CI too since OCaml-CI added 4.14 already and we're failing on it.

[gasche]

Done!

[Leonidas-from-XIV]

Looks like List.to_seq was only added in 4.07. I've opened an issue on seq to see what @c-cube thinks about adding it, but in the meantime we could just implement our own version.

[gasche]

I changed the code to avoid List.seq and rebased.

[Leonidas-from-XIV]

Seq.cons was added in 4.11, but given it is just fun x xs -> Seq.Cons (x, xs) it is an easy fix to get it to work throughout all OCaml versions.

[gasche]

Damn! Sorry for the noise, I did another attempt with just Seq.Cons and Seq.Nil.

[Leonidas-from-XIV]

Looks good, I think we got all concerns addressed. Thanks for your work @gasche!