Skip to content

Use messagepack extensions payloads that can be transmitted#16

Open
notmgsk wants to merge 5 commits intombrezu:masterfrom
notmgsk:feature/extensions
Open

Use messagepack extensions payloads that can be transmitted#16
notmgsk wants to merge 5 commits intombrezu:masterfrom
notmgsk:feature/extensions

Conversation

@notmgsk
Copy link
Contributor

@notmgsk notmgsk commented Feb 20, 2019

As far as I have been able to understand, cl-messagepack's current
implementation of extensions doesn't allow you to encode arbitrary
data, associate it with some type, and then communicate this across a
network. cl-messagepack's implementation is more like you have some
instance of an object, you assign an ID to it, you can then
communicate this ID across the network and back, and then lookup this
object in a hashtable using the ID. This restricts you to keeping the
data in the same lisp image / memory.

This is a first stab at what I think is a "true" implementation of
messagepack's extensions. It is incomplete, but I'm making the PR for
visibility.

Use it to encode complex floats with the following:

(defun my-encode-complex (data stream)
  (encode-fixext-8
   (append
    (rest (coerce (flexi-streams:with-output-to-sequence (s) (encode-float (realpart data) s)) 'list))
    (rest (coerce (flexi-streams:with-output-to-sequence (s) (encode-float (imagpart data) s)) 'list)))
   stream
   0))

(defun my-decode-complex (len stream)
  (complex (sb-kernel:make-single-float (ub32->sb32 (load-big-endian stream 4)))
           (sb-kernel:make-single-float (ub32->sb32 (load-big-endian stream 4))))
  )

(register-extension-dispatcher 0 #'complexp #'my-encode-complex #'my-decode-complex)

CL-USER> (mpk::decode (mpk::encode (list #C(1.2 1.3)
                                         #C(0.0 1.0)
                                         (list #C(1.2 1.3)))))
#(#C(1.2 1.3) #C(0.0 1.0) #(#C(1.2 1.3)))

@phmarek
Copy link
Collaborator

phmarek commented Feb 20, 2019

Hi,

thanks, this looks good. I'll try to watch your progress; but please be sure to notify me when you think you're ready!

I just pushed a commit that should make your my-encode-float much more concise.

As far as I have been able to understand, cl-messagepack's current
implementation of extensions doesn't allow you to encode arbitrary
data, associate it with some type, and then communicate this across a
network. cl-messagepack's implementation is more like you have some
instance of an object, you assign an ID to it, you can then
communicate this ID across the network and back, and then lookup this
object in a hashtable using the ID. This restricts you to keeping the
data in the same lisp image / memory.

This is a first stab at what I think is a "true" implementation of
messagepack's extensions. It is incomplete, but I'm making the PR for
visibility.

Use it to encode complex floats with the following:

```
(defun my-encode-complex (data stream)
  (encode-fixext-8
   (append
    (rest (coerce (flexi-streams:with-output-to-sequence (s) (encode-float (realpart data) s)) 'list))
    (rest (coerce (flexi-streams:with-output-to-sequence (s) (encode-float (imagpart data) s)) 'list)))
   stream
   0))

(defun my-decode-complex (len stream)
  (complex (sb-kernel:make-single-float (ub32->sb32 (load-big-endian stream 4)))
           (sb-kernel:make-single-float (ub32->sb32 (load-big-endian stream 4))))
  ;; (or #+sbcl (sb-kernel:make-single-float (ub32->sb32 (load-big-endian stream 4)))
  ;;     #-(or sbcl) (error "No floating point support yet."))
  ;; (or #+sbcl (sb-kernel:make-double-float (ub32->sb32 (load-big-endian stream 4))
  ;;                                         (load-big-endian stream 4))
  ;;     #+ccl (ccl::double-float-from-bits (load-big-endian stream 4)
  ;;                                        (load-big-endian stream 4))
  ;;     #-(or sbcl ccl) (error "No floating point support yet."))
  )

(register-extension-dispatcher 0 #'complexp #'my-encode-complex

CL-USER> (mpk::decode (mpk::encode (list #C(1.2 1.3)
                                         #C(0.0 1.0)
                                         (list #C(1.2 1.3)))))

```
@notmgsk notmgsk marked this pull request as ready for review February 25, 2019 02:37
@notmgsk
Copy link
Contributor Author

notmgsk commented Feb 25, 2019

@phmarek I think it's in a review-able state now. I've added tests. With your changes, the encoding of complex floats looks like


(defun %serialize-complex (data stream)
  (mpk::encode-fixext-8 (append (flexi-streams:with-output-to-sequence (s :as-list t) (mpk::encode-float (realpart data) s t))
                                (flexi-streams:with-output-to-sequence (s :as-list t) (mpk::encode-float (imagpart data) s t)))
                        stream
                        0))

(defun %deserialize-complex (len stream)
  (complex (sb-kernel:make-single-float (mpk::ub32->sb32 (mpk::load-big-endian stream 4)))
           (sb-kernel:make-single-float (mpk::ub32->sb32 (mpk::load-big-endian stream 4)))))

(mpk:register-extension-dispatcher 0 #'complexp #'%serialize-complex #'%deserialize-complex)

I'm not married to this interface, and would be more than happy to hear your thoughts and suggestions.

@phmarek
Copy link
Collaborator

phmarek commented Feb 25, 2019

Thanks. A few questions - but please don't just change your code, let's discuss that first.

  • REGISTER-EXTENSION-DISPATCHER has a TODO comment that I don't understand.
  • If the user-specific encoders could just return (a list of) byte vectors, then all that WITH-OUTPUT-TO-SEQUENCE etc. could be dropped. That would be much more convenient, right?
  • In case multiple libraries (or different parts of one program) need to use different encodings, some kind of (WITH-EXTENSIONS (hashtable) ...) would be nice to have; for that REGISTER-EXTENSION-DISPATCHER needs to receive another argument that defaults to the special variable (and would be initialized if NIL) (switch to keyword arguments, perhaps for the matcher/encoder/decoder as well?)
  • I don't understand DISPATCHER-FOR-EXTENSION-DATA. You use a hash table but then look at all the values? Please don't do the lookup twice -- bind a local variable to the found encoder and test it.
  • DECODER-FOR-EXTENSION-DATA isn't used at all?
  • What about compatibility with users of the old extension system?

@notmgsk
Copy link
Contributor Author

notmgsk commented Feb 25, 2019

Thanks for the feedback!

  • REGISTER-EXTENSION-DISPATCHER has a TODO comment that I don't understand.

This will be removed, and can be ignored for now.

  • If the user-specific encoders could just return (a list of) byte vectors, then all that WITH-OUTPUT-TO-SEQUENCE etc. could be dropped. That would be much more convenient, right?

Do you mean if mpk:encode-float could return a list, rather than write to a stream? That would be helpful. An alternative would be to investigate whether we could pass &optional drop-prefix to mpk:encode which is then passed on to all further mpk:encode-* functions. Then I could simply write

(defun %serialize-complex (data stream)
  (mpk::encode-fixext-16 (append (mpk::encode (realpart data) t)
                                 (mpk::encode (imagpart data) t))
                         stream
                         0))
  • In case multiple libraries (or different parts of one program) need to use different encodings, some kind of (WITH-EXTENSIONS (hashtable) ...) would be nice to have; for that REGISTER-EXTENSION-DISPATCHER needs to receive another argument that defaults to the special variable (and would be initialized if NIL) (switch to keyword arguments, perhaps for the matcher/encoder/decoder as well?)

Do you mean something that rebinds mpk::*extension-dispatchers*? That could be useful. Something like this?

(defmacro with-extensions ((extensions-ht) &body body)
  `(let ((mpk::*extension-dispatchers* ,extensions-ht))
     ,@body))
* I don't understand `DISPATCHER-FOR-EXTENSION-DATA`. You use a hash table but then look at all the values? Please don't do the lookup twice -- bind a local variable to the found encoder and test it.

The data structure used here could definitely be improved. Will think about how to do that.

* What about compatibility with users of the old extension system?

This one is tough. My sense is that the implementations are sufficiently different that they are not compatible. Maybe with some clever hacking they could be.

@phmarek
Copy link
Collaborator

phmarek commented Feb 27, 2019 via email

@notmgsk
Copy link
Contributor Author

notmgsk commented Feb 27, 2019

Hi Mark!
An alternative would be to investigate whether we could pass &optional drop-prefix to mpk:encode which is then passed on to all further mpk:encode-* functions.
That might be another idea, yes. Please investigate and get back to me whether it looks better in practice ;)

Will do.

Do you mean something that rebinds mpk::*extension-dispatchers*?
Yeah, a WITH- macro, and the registration needs to be able to write into a non-default hashtable.

Sounds reasonable.

  • What about compatibility with users of the old extension system? This one is tough. My sense is that the implementations are sufficiently different that they are not compatible. Maybe with some clever hacking they could be.
    Well, what was the reason you started anew? Perhaps we should separate the old and the new way into different files, and have them with #+mpk-marks-extensions and #-mpk-marks-extensions in the .ASD file (please note that I'm avoiding "new", as that would get old within a few years ;)

To answer the first part of that: I don't think the old implementation was generally useful. You couldn't for example encode arbitrary data, send it over a network to a completely separate process, and then decode it -- which is a problem that I think messagepack's extensions were meant to solve.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants