WIP: annotated decoder #4949

crocodile-dentist · 2024-09-05T09:55:11Z

todo: tests

Description

Annotated decoder

Checklist

Quality

Commit sequence makes sense and have useful messages, see ref.
New tests are added and existing tests are updated.
Self-reviewed the PR.

Maintenance

Linked an issue or added the PR to the current sprint of ouroboros-network project.
Added labels.
Updated changelog files.
The documentation has been properly updated, see ref.

crocodile-dentist · 2024-09-05T10:16:35Z

Having a single runDecoderWithChannel and runDecoderWithLimit avoids some code duplication to ease maintenance, but to avoid leaking space for codec's that do not need annotation capability there should probably be a custom monoid instance for the bytes type which is strict in the accumulator and essentially a no-op.

coot · 2024-09-05T10:19:36Z

ouroboros-network-api/src/Ouroboros/Network/SizeInBytes.hs

@@ -14,6 +18,13 @@ import Data.Measure qualified as Measure
 import NoThunks.Class (NoThunks (..))
 import Quiet (Quiet (..))

+data WithBytes a = WithBytes { wbValue    :: !a,
+                               unannotate :: ShortByteString }


If we use ShortByteString this means we have to copy the bytestring on the heap, isn't it. With Lazy.ByteString we won't have this problem.

Yes, but if we have annotations floating around in regular bytestrings, it can cause heap fragmentation because they are pinned.

coot · 2024-09-05T10:29:33Z

ouroboros-network-framework/src/Ouroboros/Network/Driver/Simple.hs

-runAnnotatedPeer
-  :: forall ps (st :: ps) pr failure bytes m a .
-     ( MonadThrow m
     , Monoid bytes
-     , Show failure
-     , forall (st' :: ps). Show (ClientHasAgency st')
-     , forall (st' :: ps). Show (ServerHasAgency st')
-     , ShowProxy ps
     )
  => Tracer m (TraceSendRecv ps)
-  -> AnnotatedCodec ps failure m bytes
+  -> Codec ps failure m annotator bytes
  -> Channel m bytes
+  -> (forall st. annotator st -> bytes -> SomeMessage st)
  -> Peer ps pr st m a
  -> m (a, Maybe bytes)
-runAnnotatedPeer tracer codec channel peer =
+runPeer tracer codec channel runAnnotator peer =
    runPeerWithDriver driver peer (startDState driver)
  where
-    driver = annotatedSimpleDriver tracer codec channel
+    driver = mkSimpleDriver runAnnotator tracer codec channel


I thought that exposing the original runPeer makes sense if we think about integration: all, but one protocols, don't need annotator right now.

coot · 2024-09-05T10:31:43Z

ouroboros-network-framework/src/Ouroboros/Network/Mux.hs

     )
  => (ctx -> ( Tracer m (TraceSendRecv ps)
-             , Codec ps failure m bytes
+             , Codec ps failure m annotator bytes
+             , forall st. annotator st -> bytes -> SomeMessage st


ugh it needs to show up in a lot of places now. Is it worth it. It exposes internal details of how the peer is run. Maybe we've gone too far.

Yes, I've noticed when I was done 😅 , but for presentation purposes just decided to show it as is.

coot · 2024-09-05T10:32:58Z

ouroboros-network-protocols/src/Ouroboros/Network/Protocol/LocalTxSubmission/Codec.hs

-  -> Codec (LocalTxSubmission tx reject) CBOR.DeserialiseFailure m ByteString
+  -> Codec (LocalTxSubmission tx reject) CBOR.DeserialiseFailure m SomeMessage ByteString


This is also a bad consequence that we need to modify all our codes, for the purpose of adding a feature to just one of them.

coot · 2024-09-05T10:51:21Z

ouroboros-network-protocols/src/Ouroboros/Network/Protocol/TxSubmission2/Codec.hs

+                                SomeMessage . MsgReplyTxs $
+                                  txs <&> \(tx, start, end) ->
+                                             let slice = BSL.take (end - start ) $ BSL.drop start bytes
+                                             in WithBytes {
+                                               wbValue = tx,
+                                               unannotate = toShort . BSL.toStrict $ slice })


Remeber that we decided to pass a tx -> SizeInBytes to the inbound side of tx-submission. So we don't really need to change things here.

And if we did accept this codec change, then I'd use WithBytes tx everywhere, then we don't need to modify Protocol instance and pollute it with unrelated changes (e.g. the WithBytes tx just in a single place). As a side effect we could take advantage of unannotate destructor as an encoder.

Note that if we stick with Lazy.ByteString then we won't need toShort . BSL.toStrict here at all. All chunks of bytes are evaluated anyway on the heap, maybe just the spine of the lazy bytestring bytes isn't, since we concat it from multiple fully evaluated pieces.

This was written for demo purposes so certain compromises were made in terms of WithBytes tx, but yes we will use the tx -> SizeInBytes for the actual implementation.

crocodile-dentist requested a review from a team as a code owner September 5, 2024 09:55

crocodile-dentist force-pushed the mwojtowicz/annotated-decoder branch from 03dfec1 to b61e8b1 Compare September 5, 2024 10:05

crocodile-dentist force-pushed the mwojtowicz/annotated-decoder branch from b61e8b1 to 7b5e1e1 Compare September 5, 2024 10:44

coot reviewed Sep 5, 2024

View reviewed changes

coot force-pushed the coot/annotated-decoder branch from 4b47e98 to fede497 Compare September 5, 2024 12:26

crocodile-dentist force-pushed the mwojtowicz/annotated-decoder branch 3 times, most recently from 4a0b1b5 to 2b90a63 Compare September 6, 2024 15:29

Annotated decoder

2fa42cb

crocodile-dentist force-pushed the mwojtowicz/annotated-decoder branch from 2b90a63 to 2fa42cb Compare September 10, 2024 09:58

coot changed the title ~~WIP~~ WIP: annotated decoder Sep 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: annotated decoder #4949

WIP: annotated decoder #4949

crocodile-dentist commented Sep 5, 2024

crocodile-dentist commented Sep 5, 2024

coot Sep 5, 2024

crocodile-dentist Sep 5, 2024

coot Sep 5, 2024

crocodile-dentist Sep 5, 2024

coot Sep 5, 2024

crocodile-dentist Sep 5, 2024

coot Sep 5, 2024

coot Sep 5, 2024

crocodile-dentist Sep 5, 2024

		-> Codec (LocalTxSubmission tx reject) CBOR.DeserialiseFailure m ByteString
		-> Codec (LocalTxSubmission tx reject) CBOR.DeserialiseFailure m SomeMessage ByteString

WIP: annotated decoder #4949

Are you sure you want to change the base?

WIP: annotated decoder #4949

Conversation

crocodile-dentist commented Sep 5, 2024

Description

Checklist

Quality

Maintenance

crocodile-dentist commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment