Faster binary encoding/decoding #1030

samoht · 2020-07-10T17:36:31Z

samoht · 2020-07-11T11:18:08Z

I've removed the confusing headers option and added an explicit Unboxed sub-module for unboxed binary operations. I'll merge once the CI is green.

craigfe · 2020-07-11T12:18:47Z

I can review on Monday, if needed

craigfe

LGTM. Reviews are mostly nits. I think the staging + explicit Unboxed APIs are much nicer to use (also has the advantage that we no longer rely on optional argument erasure in a way that breaks ppx_bisect).

CHANGES.md

craigfe · 2020-07-12T13:30:49Z

src/irmin/node.ml

+(* OK: ASSERT node to_bin_string:
+   "
+\000\000\000\000\000\000\000\002
+\000\000\000\000\000\000\000\003foo
+\255\000\000\000\000\000\000\000
+\020\005\254@WS\022o\018UY\231\201\172U\134T\241\007\199\233
+\000\000\000\000\000\000\000\000\000\003bar
+\000\000
+\255\000\000\000\000\000\000\000
+\020:\245\172\213`vT\179\167\031\015\158\018\195\026\023i\025s&"
+
+KO: ASSERT node to_bin_string:
+   "
+\000\000\000\000\000\000\000\002
+\000\000\000\000\000\000\000\003foo
+\255\000\000\000\000\000\000\000
+\020\2189\163\238^kK\r2U\191\239\149`\024\144\175\216\007\t
+\000\000\000\000\000\000\000\000\000\003bar
+\000\000
+\255\000\000\000\000\000\000\000
+\020b\205\183\002\015\249\229\170d,=@f\149\r\209\240\031M" *)


Accidental comment slipped in here?

craigfe · 2020-07-12T20:06:53Z

test/irmin/test_type.ml

@@ -70,7 +111,8 @@ let ok x = Alcotest.result x error
 let test_json () =
  let s = T.to_json_string hex "foo" in
  Alcotest.(check string) "JSON hex" "\"666f6f\"" s;
-  let s = T.to_bin_string hex "foo" in
+  let s = to_bin_string hex "foo" in
+  Fmt.epr "XXX %S\n%!" s;


Looks like a stray debug statement.

craigfe · 2020-07-12T20:23:49Z

src/irmin/type/type.mli

@@ -446,37 +460,49 @@ val size_of : 'a t -> 'a size_of
 (** [size_of t x] is either the size of [encode_bin t x] or the binary encoding
    of [x], if the backend is not able to pre-compute serialisation lengths. *)

+module Unboxed : sig


Perhaps some documentation here explaining the optimisation being performed.

craigfe · 2020-07-12T20:26:58Z

src/irmin/type/type_binary.ml

-      if ofs = 0 && len = String.length buf then ok len (Bytes.of_string buf)
+  let mk_boxed of_string of_bytes n =
+    let sub len buf ofs =
+      if ofs = 0 && len = String.length buf then (len, of_string buf)
      else
        let str = Bytes.create len in
        String.blit buf ofs str 0 len;


Not related to this PR, but it might be worth failing more gracefully here in the case len > String.length buf (which corresponds to trying to read a header that doesn't exist / malformed length header). That's quite easy to enounter as a user of Type.Unboxed.

src/irmin/type/type_binary.ml

craigfe · 2020-07-12T20:51:09Z

src/irmin/type/type_size.ml

+let unboxed_string = function
+  | `Fixed len -> fun _ -> len (* fixed-size strings are never boxed *)
+  | _ -> String.length


Not related to this PR, but I think it's unintuitive that Type.(string_of (`Fixed 8))` and `Type.(string_of `Int64) are not the same w.r.t. this function (since the corresponding string doesn't need to be of the correct length). I think we should make a consistent choice here, but we could also just say that it's undefined.

craigfe · 2020-07-12T20:53:19Z

test/irmin/test_type.ml

+  let check msg ty =
+    let msg f = Fmt.strf "%s: %s" msg f in
+    let buf = with_buf (encode_bin ty foo) in
+    Alcotest.(check string) (msg "boxed") buf "\003foo";
+    let buf = with_buf (Unboxed.encode_bin ty foo) in
+    Alcotest.(check string) (msg "unboxed") buf "foo";
+    let buf = with_buf (Unboxed.encode_bin (T.boxed ty) foo) in
+    Alcotest.(check string) (msg "force boxed") buf "\003foo"
+  in
+  check "string" T.string;
+  check "like" (T.like T.string);
+  check "map" (T.map T.string id id)


Can we just duplicate these tests for T.bytes?

samoht · 2020-07-13T10:43:20Z

I've tried to take all your comments into account (but the 2 about the len which can be addressed later -- as I am not sure how to address these properly)

- add staging to all binary encoding/decoding operations - replace the confusing ?headers argument by a specialized Type.Unboxed module - pre-alocate closures in generic operation as early as possible, whenever the type is known

craigfe

Merge when ready.

I'll create a separate issue about the length issues.

samoht · 2020-07-15T07:17:52Z

CI restarted...

samoht force-pushed the faster-binary-encoding branch from 4a7f75a to ba5675f Compare July 11, 2020 11:04

craigfe reviewed Jul 12, 2020

View reviewed changes

samoht force-pushed the faster-binary-encoding branch from 35d864e to 8190345 Compare July 13, 2020 10:41

samoht added 4 commits July 13, 2020 12:45

Irmin.Type: more efficient binary serialisation

b668204

- add staging to all binary encoding/decoding operations - replace the confusing ?headers argument by a specialized Type.Unboxed module - pre-alocate closures in generic operation as early as possible, whenever the type is known

Faster Type.{equal,compare}

b44073e

Faster Type.{*json*} functions

92e89c1

Update CHANGES

41b471c

samoht force-pushed the faster-binary-encoding branch from 8190345 to 41b471c Compare July 13, 2020 10:45

craigfe approved these changes Jul 13, 2020

View reviewed changes

This was referenced Jul 13, 2020

Review handling of fixed-length types and Type.size_of #1037

Closed

Irmin.Type should be more careful when unmarshalling random data #483

Closed

samoht mentioned this pull request Jul 13, 2020

irmin-indexeddb: add a conflict with irmin's master ocaml/opam-repository#16808

Merged

craigfe merged commit bfcc2e3 into mirage:master Jul 15, 2020

samoht deleted the faster-binary-encoding branch July 15, 2020 08:14

craigfe mentioned this pull request Sep 28, 2020

irmin.type: further improve staging of generic operations #1093

Merged

craigfe mentioned this pull request Oct 14, 2020

Review handling of fixed-length types and Type.size_of mirage/repr#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Faster binary encoding/decoding #1030

Faster binary encoding/decoding #1030

samoht commented Jul 10, 2020

samoht commented Jul 11, 2020

craigfe commented Jul 11, 2020

craigfe left a comment

craigfe Jul 12, 2020

craigfe Jul 12, 2020

craigfe Jul 12, 2020

craigfe Jul 12, 2020

craigfe Jul 12, 2020

craigfe Jul 12, 2020

samoht commented Jul 13, 2020

craigfe left a comment

samoht commented Jul 15, 2020

Faster binary encoding/decoding #1030

Faster binary encoding/decoding #1030

Conversation

samoht commented Jul 10, 2020

samoht commented Jul 11, 2020

craigfe commented Jul 11, 2020

craigfe left a comment

Choose a reason for hiding this comment

craigfe Jul 12, 2020

Choose a reason for hiding this comment

craigfe Jul 12, 2020

Choose a reason for hiding this comment

craigfe Jul 12, 2020

Choose a reason for hiding this comment

craigfe Jul 12, 2020

Choose a reason for hiding this comment

craigfe Jul 12, 2020

Choose a reason for hiding this comment

craigfe Jul 12, 2020

Choose a reason for hiding this comment

samoht commented Jul 13, 2020

craigfe left a comment

Choose a reason for hiding this comment

samoht commented Jul 15, 2020