Parsing using the Storable (Data.Store) #20
Conversation
Implemented the Storable typeclass from Data.Store. Was implemented in such a way that all the machinery from Data.Store can be used to efficiently encode and decode the Wayland ByteString format.
| } | ||
| {-# INLINE size #-} | ||
| {-# INLINE peek #-} | ||
| {-# INLINE poke #-} |
There was a problem hiding this comment.
I was going to complain about these pragmas, but store itself seems to have them as well, so I suppose we should add a comment saying that we just follow those conventions.
Also, perhaps future readers might be confused for a second why we can't just peek the payload. So perhaps a comment saying they're variable size.
(There seems to be support in store for ByteStrings using some StaticSize type, but that's probably overkill for us.)
| @@ -944,11 +945,11 @@ display_read_events proxyP = withStablePtr proxyP $ \proxy -> do | |||
| fd_queue <- newTQueueIO | |||
| atomically $ mapM_ (writeTQueue fd_queue) fds | |||
|
|
|||
| let pkgsParse = parseOnly pkgStream bytes | |||
| let pkgsParse = (S.decode bytes :: Either S.PeekException [WirePackage]) | |||
There was a problem hiding this comment.
Where does the Store instance of [WirePackage] come from? As the latter is variable size...
There was a problem hiding this comment.
I think you can drop the parentheses here.
| sz <- peek | ||
| let payloadSize = fromIntegral sz - 8 | ||
| plraw <- peekToPlainForeignPtr "Data.ByteString.ByteString" payloadSize | ||
| let pl = B.PS plraw 0 payloadSize |
There was a problem hiding this comment.
I don't understand if this is able to leak memory or not. And in any case, these two lines are strange, as peekToPlainForeignPtr creates a ByteString, and then extracts the ForeignPtr, which we then reinsert into a ByteString. Surely we can do this directly. If not, please add a comment explaining why not.
| -- Allows us to use the optimized machinery for Data.Store | ||
| -- to gain faster performing decoding/encoding. | ||
| instance Store WirePackage where | ||
| size = VarSize $ fromIntegral . wirePackageSize |
There was a problem hiding this comment.
(perhaps we should add test cases that check that indeed the byte size of a WirePackage is its wirePackageSize - though perhaps that is a store invariant)
| wirePackBuilder sender size opcode payload = | ||
| BBE.word32Host sender | ||
| <> | ||
| -- FIXME make byte order portable here |
There was a problem hiding this comment.
I forgot what this FIXME was about, it seems to be doing the right thing. So yes this comment can probably be dropped.
| payload | ||
| -- Storable that writes in the Wayland binary format. | ||
| -- Allows us to use the optimized machinery for Data.Store | ||
| -- to gain faster performing decoding/encoding. |
There was a problem hiding this comment.
This is a useful comment, so make it a haddock one by starting with -- |. "Faster performing" than what?
| { wirePackageSender=sen | ||
| , wirePackageSize=sz | ||
| , wirePackageOpcode=op | ||
| , wirePackagePayload=pl |
|
Upon further investigation, it seems that two points of my review are ill-informed: you're just using store's own approach to implementing a |
| pkgStream :: A.Parser [WirePackage] | ||
| pkgStream = A.many' parseWirePackage | ||
| wirePack :: WirePackage -> BB.Builder | ||
| wirePack = BB.byteString . encode |
There was a problem hiding this comment.
I suppose we can replace wirePack by the list instance of Store: its only purpose is to sequences packages into a big binary blob (see serializeQueue1 in Graphics.Sudbury.CABI.Common), and store can do this for us.
There was a problem hiding this comment.
(but then we'd have to first use this list instance, and then cut away the first value in the output which is the length)
There was a problem hiding this comment.
So currently this breaks regardless due to the fact that Store doesn't align on 32 bits, which the Wayland protocol requires (I'll include a test for future reference). I think the easiest fix would be a newtype wrapper around a list and write an override for the store implementation (which would be identical for the most part to the default list implementation in Store).
The list instances used previously were incorrect as the dont account for padding and do not have the correct format (prepend the size of the list). Added a test case (with a multiple packages bytestring arbitrary instance) to check for this in the future.
I have not yet ran these changes against a compositor. Will do so soon!
Implemented the Storable typeclass from Data.Store. Was implemented in
such a way that all the machinery from Data.Store can be used to
efficiently encode and decode the Wayland ByteString format.
Fixes #6.