search

crystal-community / msgpack-crystal

MessagePack implementation in Crystal msgpack.org[Crystal]
136 stars 18 forks source link

WIP: Optional zero copy slices when unpacking #67

Closed didactic-drunk closed 3 years ago

didactic-drunk commented 3 years ago

Worst results from a handful of runs:

> 250x speedup for large `String`
> 2x speedup for `Array(String)`

One sample:

big binary = 2000000000, 00:00:00.889487331
big binary(zc) = 2000000000, 00:00:00.001716480
hash string binary = 10010000, 00:00:02.700008714
hash string binary(zc) = 10010000, 00:00:02.454739566
array of binaries = 10000000, 00:00:00.883896397
array of binaries(zc) = 10000000, 00:00:00.393608187

zero_copy is kept as an explicitly specified param

  1. As a warning at all call sites that memory is shared
  2. To allow a future more common param to have position 2 or 3

Tests will come when the design is approved.

It would be nice to have a (compile time) standard interface for querying an IO for zero copying ability. An mmapped File would also be capable of zc but this implementation doesn't allow it. Perhaps has_method?(:to_slice)?

kostya commented 3 years ago

Thanks. I think this zero_copy option everywhere looks quite weird, may be add new ZeroCopyIoUnpacker < IOUnpacker class, and also ZeroCopyLexer < Lexer. And this zero_copy option would be only in from_msgpack method.

didactic-drunk commented 3 years ago

@kostya I'm not sure if this is what your going for but there are problems.

I think your request requires lots of def new(x : SpecificType) for [String, Bytes, Array(UInt8), IO::Memory (on the zc subclasses)] but those the additional new's aren't available in subclasses. If you have a way of handling this please commit.

zero_copy was changed to it's own set of methods from a param in an attempt to remove virtual method calls and get compile time type safety.

Currently broken:

kostya commented 3 years ago

i added in branch pr_67 fixes to your branch (https://github.com/crystal-community/msgpack-crystal/commit/7a8172937a4796e54ef964a52ffe4c756657ac82), please apply them. I guess def new is Crystal bug (https://github.com/crystal-lang/crystal/issues/10804), and until it fixed IOUnpackerZeroCopy copy all self.new methods.