batteries' input_line is slower than the stdlib's one #520

Open
UnixJunkie opened this Issue Jan 29, 2014 · 12 comments

Comments

Projects
None yet
6 participants
@UnixJunkie
Member

UnixJunkie commented Jan 29, 2014

I can read a 160041 lines file ~4.9 times
faster using Legacy.input_line...

@c-cube

This comment has been minimized.

Show comment
Hide comment
@c-cube

c-cube Jan 29, 2014

Member

I think we need many more benchmarks...

Member

c-cube commented Jan 29, 2014

I think we need many more benchmarks...

@UnixJunkie

This comment has been minimized.

Show comment
Hide comment
@UnixJunkie

UnixJunkie Jan 29, 2014

Member

The culprit looks like BatIO and Enum, that add some wrapping around each call to
read_line.

Member

UnixJunkie commented Jan 29, 2014

The culprit looks like BatIO and Enum, that add some wrapping around each call to
read_line.

@agarwal

This comment has been minimized.

Show comment
Hide comment
@agarwal

agarwal Jan 29, 2014

Member

You might be interested in this thread [1] on the ocsigen list too. Some of
the ideas are not lwt specific.

On Wed, Jan 29, 2014 at 4:08 AM, Francois Berenger <notifications@github.com

wrote:

The culprit looks like BatIO and Enum, that add some wrapping around each
call to
read_line.

Reply to this email directly or view it on GitHubhttps://github.com/ocaml-batteries-team/batteries-included/issues/520#issuecomment-33567114
.

Member

agarwal commented Jan 29, 2014

You might be interested in this thread [1] on the ocsigen list too. Some of
the ideas are not lwt specific.

On Wed, Jan 29, 2014 at 4:08 AM, Francois Berenger <notifications@github.com

wrote:

The culprit looks like BatIO and Enum, that add some wrapping around each
call to
read_line.

Reply to this email directly or view it on GitHubhttps://github.com/ocaml-batteries-team/batteries-included/issues/520#issuecomment-33567114
.

@hcarty

This comment has been minimized.

Show comment
Hide comment
@hcarty

hcarty Jan 29, 2014

Contributor

@agarwal I think you missed the link in your comment.

Contributor

hcarty commented Jan 29, 2014

@agarwal I think you missed the link in your comment.

@agarwal

This comment has been minimized.

Show comment
Hide comment
@agarwal

agarwal Jan 29, 2014

Member

Sorry. Here it is:

[1] https://sympa.inria.fr/sympa/arc/ocsigen/2013-12/msg00001.html

On Wed, Jan 29, 2014 at 10:35 AM, Hezekiah M. Carty <
notifications@github.com> wrote:

@agarwal https://github.com/agarwal I think you missed the link in your
comment.

Reply to this email directly or view it on GitHubhttps://github.com/ocaml-batteries-team/batteries-included/issues/520#issuecomment-33595086
.

Member

agarwal commented Jan 29, 2014

Sorry. Here it is:

[1] https://sympa.inria.fr/sympa/arc/ocsigen/2013-12/msg00001.html

On Wed, Jan 29, 2014 at 10:35 AM, Hezekiah M. Carty <
notifications@github.com> wrote:

@agarwal https://github.com/agarwal I think you missed the link in your
comment.

Reply to this email directly or view it on GitHubhttps://github.com/ocaml-batteries-team/batteries-included/issues/520#issuecomment-33595086
.

@c-cube

This comment has been minimized.

Show comment
Hide comment
@c-cube

c-cube Jan 29, 2014

Member

I looked at the code, and the source of slowness is probably twofold:

  • the overhead of Enum combinators (including Enum.suffix_action that is only useful for the last element, but still wraps every call to Enum.next)
  • assuming @UnixJunkie used BatIO.lines_of (a very convenient combinator indeed), it just calls read_line repeatedly instead of, say, allocating one buffer and re-using it
Member

c-cube commented Jan 29, 2014

I looked at the code, and the source of slowness is probably twofold:

  • the overhead of Enum combinators (including Enum.suffix_action that is only useful for the last element, but still wraps every call to Enum.next)
  • assuming @UnixJunkie used BatIO.lines_of (a very convenient combinator indeed), it just calls read_line repeatedly instead of, say, allocating one buffer and re-using it
@UnixJunkie

This comment has been minimized.

Show comment
Hide comment
@UnixJunkie

UnixJunkie Jan 30, 2014

Member

I used File.lines_of, but yes then at some point it calls BatIO.lines_of.

Member

UnixJunkie commented Jan 30, 2014

I used File.lines_of, but yes then at some point it calls BatIO.lines_of.

@UnixJunkie

This comment has been minimized.

Show comment
Hide comment
@UnixJunkie

UnixJunkie Jan 31, 2014

Member

I think I should close this (since I created it): this is a too old performance regression compared to the stdlib
that was introduced too long time ago.

Member

UnixJunkie commented Jan 31, 2014

I think I should close this (since I created it): this is a too old performance regression compared to the stdlib
that was introduced too long time ago.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Jan 31, 2014

Member

I don't think there is anything wrong with fixing old issues. c-cube looks interested in the performance aspect of BatIO (I personally tend to loathe IO-related stuff, so I should apologize for happily staying away of the discussion). It would help to have your actual benchmark code, though -- I would guess that most reasons we suspect could cause this regression will turn out not to matter that much in a realistic workflow, with one actual suspect being guitly by a large margin.

Thank you for looking at this!

Member

gasche commented Jan 31, 2014

I don't think there is anything wrong with fixing old issues. c-cube looks interested in the performance aspect of BatIO (I personally tend to loathe IO-related stuff, so I should apologize for happily staying away of the discussion). It would help to have your actual benchmark code, though -- I would guess that most reasons we suspect could cause this regression will turn out not to matter that much in a realistic workflow, with one actual suspect being guitly by a large margin.

Thank you for looking at this!

@c-cube

This comment has been minimized.

Show comment
Hide comment
@c-cube

c-cube Jan 31, 2014

Member

Yet another argument to put IO in a separate library imho: if people want to use batteries in combination with libraries that use the standard input and ouput channels (and know what happens underneath, IO contains some bloat, like weak sets and whatnot).

Member

c-cube commented Jan 31, 2014

Yet another argument to put IO in a separate library imho: if people want to use batteries in combination with libraries that use the standard input and ouput channels (and know what happens underneath, IO contains some bloat, like weak sets and whatnot).

@rgrinberg

This comment has been minimized.

Show comment
Hide comment
@rgrinberg

rgrinberg Jan 31, 2014

Member

Yep. I'm also pretty sure that IO brings in Unix. Separating the core of batteries from Unix is one of the main goals of refactoring.

Member

rgrinberg commented Jan 31, 2014

Yep. I'm also pretty sure that IO brings in Unix. Separating the core of batteries from Unix is one of the main goals of refactoring.

@UnixJunkie

This comment has been minimized.

Show comment
Hide comment
@UnixJunkie

UnixJunkie Feb 3, 2014

Member

Implementing a wc -l in ocaml with batteries (File.lines_of) is enough to see the problem.
The same program using (Legacy.open_in, Legacy.input_line, Legacy.close_in) will be faster I bet.
Here is my version trying to avoid batteries' IO:

open Batteries
open Printf

module MU = My_utils

let with_in_file fn f =
  let input = Legacy.open_in fn in
  let res = f input in
  Legacy.close_in input;
  res

let main () =

  let nb_lines = ref 0 in

  let _all_lines =
    with_in_file Sys.argv.(1) (fun input ->
      let res, _eof =
        MU.unfold_exc
          (fun () -> let l = Legacy.input_line input in
                     incr nb_lines;
                     l
          )
      in
      res
    )
  in
  printf "init %d lines\n" !nb_lines;
;;

main ()

MU.unfold_exc is the new constructor I am pushing for in BatList.

Member

UnixJunkie commented Feb 3, 2014

Implementing a wc -l in ocaml with batteries (File.lines_of) is enough to see the problem.
The same program using (Legacy.open_in, Legacy.input_line, Legacy.close_in) will be faster I bet.
Here is my version trying to avoid batteries' IO:

open Batteries
open Printf

module MU = My_utils

let with_in_file fn f =
  let input = Legacy.open_in fn in
  let res = f input in
  Legacy.close_in input;
  res

let main () =

  let nb_lines = ref 0 in

  let _all_lines =
    with_in_file Sys.argv.(1) (fun input ->
      let res, _eof =
        MU.unfold_exc
          (fun () -> let l = Legacy.input_line input in
                     incr nb_lines;
                     l
          )
      in
      res
    )
  in
  printf "init %d lines\n" !nb_lines;
;;

main ()

MU.unfold_exc is the new constructor I am pushing for in BatList.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment