jangko / msgpack4nim

MessagePack serializer/deserializer implementation for Nim / msgpack.org[Nim]
http://msgpack.org/
119 stars 21 forks source link

Missing tests #18

Closed pb-cdunn closed 6 years ago

pb-cdunn commented 6 years ago

I see no tests for pack_type/unpack_type.

I need to see those so I can understand what I'm doing wrong. I have this:

proc unpack_type*(ss: MsgStream, x: var str9) =
  var str: string
  ss.unpack(str)
  copyMem(addr x[0], addr str[0], 9)

And I get this:

falcon/rr_hctg_track.nim(65, 5) Error: type mismatch: got <MsgStream, string>
but expected one of:
proc unpack[T](data: string; val: var T)
  first type mismatch at position: 1
  required type: string
  but expression 'ss' is of type: MsgStream

It worked with older msgpack and nim-0.18.0. I have tried this:

-proc unpack_type*(ss: streams.Stream, x: var str9) =
+proc unpack_type*(ss: MsgStream, x: var str9) =
   var str: string
-  msgpack4nim.unpack(ss, str)
+  ss.unpack(str)
   copyMem(addr x[0], addr str[0], 9)
pb-cdunn commented 6 years ago

Missing in examples/ too. I see this:

 23 s.pack(x) #here the magic happened
 24
 25 s.setPosition(0)
 26 var xx: CustomType
 27 s.unpack(xx) #and here too

I will try that. But I don't see how to provide (un)pack_type() for any new type.

pb-cdunn commented 6 years ago
403   let rtn = tr_stage1(infile, fn, min_len, bestn)
...
412   var ss = streams.newFileStream(fn_rtn, system.fmWrite)
413   defer: ss.close()
415   msgpack4nim.pack(ss, rtn)

I am unable to get it working with the new magic (and I'm not convinced the magic would work for me anyway, since I have some raw data).

falcon/rr_hctg_track.nim(415, 19) Error: type mismatch: got <FileStream, myprioritytable>
but expected one of:
proc pack[T](s: var MsgStream; val: T)
  first type mismatch at position: 1
  required type: var MsgStream
  but expression 'ss' is of type: FileStream
proc pack[T](val: T): string
  first type mismatch at position: 1
  required type: T
  but expression 'ss' is of type: FileStream
pb-cdunn commented 6 years ago

82de886 works fine for me, without any changes to my code. Please help me to upgrade my code.

Otherwise, I may have to lock on that older version.

jangko commented 6 years ago

please try this, looks like you're missing the var modifier. The decision to change the Stream to MsgStream is for performance reason. Sorry if the documentation is not updated properly.

proc unpack_type*(ss: var MsgStream, x: var str9) =
  var str: string
  ss.unpack(str)
  copyMem(addr x[0], addr str[0], 9)

for your second example:

403   let rtn = tr_stage1(infile, fn, min_len, bestn)
...
412   var ss = streams.newFileStream(fn_rtn, system.fmWrite)
413   defer: ss.close()
      var msgss = initMsgStream() #you can pass some buffer capacity here
415   msgpack4nim.pack(msgss, rtn)
      # later when you need to write to file
      ss.write(msgss.data)

I know this pattern is less convenience than previous version, but the runtime performance is better.

jangko commented 6 years ago

my long term goal is to use Nim's "concepts" so this library can use whatever stream compatible object, but for the moment, Nim's "concepts" is plague with bugs. while waiting for more stable "concepts", I prefer performance than convenience, because msgpack4nim is also used for rpc intensive application

pb-cdunn commented 6 years ago

Yeah, I too lament the problems with concepts.

pb-cdunn commented 6 years ago
falcon/rr_hctg_track.nim(413, 19) template/generic instantiation from here
msgpack4nim/msgpack4nim.nim(667, 30) Error: undeclared field: 'data'
        s.pack_type undistinct(field)
                               ^
412   var msgss = initMsgStream()
413   msgpack4nim.pack(msgss, rtn)

That didn't quite work. Ideas?

pb-cdunn commented 6 years ago

rtn is of type

tables.initTable[string, binaryheap.Heap[mytuple]]

https://github.com/bio-nim/nim-heap

Kind of complex. But this used to work fine.

jangko commented 6 years ago

add import msgpack4collection Nim standard library collections pack-unpack are moved to msgpack4collection, they are no longer part of codec core

pb-cdunn commented 6 years ago

I have compiled pack() on a MsgStream from a Nim stream now. But what's the equivalent for unpack()?

Your front-page docs really need to show this. Nobody wants to learn the details of MsgStream. I don't mind the intermediate data-structure; I'm all for efficiency. I just need an example.

pb-cdunn commented 6 years ago
let infile = streams.newFileStream(fn_rtn, system.fmRead)
defer:
    infile.close()
var msgss = msgpack4nim.initMsgStream()
msgss.data = infile.readAll()

That works. (I'd rather you provide something to interact with Nim standard Streams, so I don't have to access internals of MsgStream.)

But now I see a problem. One of the reasons to use msgpack format is that it very easily allows low-overhead reading of large datafiles. Each msgpack "thing" tells you in advance how much bytes to read.

But you are forcing me to read the entire string into memory! That's a significant problem.

I'm fine with having a mode that's particularly time-efficient, but I also need a mode that is memory-efficient.

I guess it's not a huge problem, since it only doubles memory at most (since the data will exist in memory after parsing anyway). But could you discuss the trade-off and why you made it?

jangko commented 6 years ago

i am not aware of your situation, I thought of common msgpack usage when I made those changes. usually msgpack usage is memory to memory, or process to process communication, and seldom involve physical file. But after I see your use case, I think I must revisit my long term goal to provide a generic interface that can be tuned to various use case and finer control of decoding step.

pb-cdunn commented 6 years ago

usually msgpack usage is memory to memory

I worked with msgpack at Amazon on the service security layer. (If you've shopped at Amazon, you have used msgpack on a stream.) Believe me; this is a common use-case.

Anyway, it's only a factor of 2x, not a big deal, memory-wise.

However, an example of how to convert between MsgStream and Nim streams is a big deal. We shouldn't be going into the internals of MsgStream, I think.

jangko commented 6 years ago

right, I assume you want something like this

var s = initMsgStream(someStream)
pb-cdunn commented 6 years ago

Hmm. That's probably good enough, if you want to add state to MsgStream. (We need both directions.)

Or without changing MsgStream, simply provide a function which takes both a MsgStream and and Nim stream and writes to the stream, and another function which reads from a stream. That's just to hide the implementation details of MsgStream.

pb-cdunn commented 6 years ago

Ugh. I thought I had this working.

falcon/rr_hctg_track.nim(414, 19) template/generic instantiation from here
/localdisk/scratch/cdunn/repo/fork/repos/nim-falcon/repos/msgpack4nim/msgpack4nim.nim(1031, 53) template/generic instantiation from here
/localdisk/scratch/cdunn/repo/fork/repos/nim-falcon/repos/msgpack4nim/msgpack4collection.nim(38, 4) template/generic instantiation from here
/localdisk/scratch/cdunn/repo/fork/repos/nim-falcon/repos/msgpack4nim/msgpack4nim.nim(657, 6) template/generic instantiation from here
/localdisk/scratch/cdunn/repo/fork/repos/nim-falcon/repos/msgpack4nim/msgpack4nim.nim(699, 32) Error: undeclared field: 'data'
          s.pack_type undistinct(field)
                                 ^

What could cause that? (I'm using nim/0.18.1)

pb-cdunn commented 6 years ago

This version of msgpack4nim worked: c95e495aeb134509e9ca5beaaef890472e891258

So I guess I need to alter my code again? This is what I had:

jangko commented 6 years ago

sorry for all the trouble, msgpack4nim now can works with stringstream, filestream, and msgstream. basically you can use your previous code again, but you still need to import msgpack4collection. while the current situation looks like you need to remove the 'var' modifier from 'var MsgStream' line 64-73, because MsgStream now is a ref object. it is not clear why nim cannot call pack_type with 'var MsgStream' for ref object.

EDIT: you can also use generic version of pack_type/unpack_type, you can see the example in readme.md#example