-
Notifications
You must be signed in to change notification settings - Fork 87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: annotated decoder #4949
base: coot/annotated-decoder
Are you sure you want to change the base?
WIP: annotated decoder #4949
Conversation
03dfec1
to
b61e8b1
Compare
Having a single |
b61e8b1
to
7b5e1e1
Compare
@@ -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 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but if we have annotations floating around in regular bytestrings, it can cause heap fragmentation because they are pinned.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that exposing the original runPeer
makes sense if we think about integration: all, but one protocols, don't need annotator right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see now.
) | ||
=> (ctx -> ( Tracer m (TraceSendRecv ps) | ||
, Codec ps failure m bytes | ||
, Codec ps failure m annotator bytes | ||
, forall st. annotator st -> bytes -> SomeMessage st |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I've noticed when I was done 😅 , but for presentation purposes just decided to show it as is.
-> Codec (LocalTxSubmission tx reject) CBOR.DeserialiseFailure m ByteString | ||
-> Codec (LocalTxSubmission tx reject) CBOR.DeserialiseFailure m SomeMessage ByteString |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
4b47e98
to
fede497
Compare
4a0b1b5
to
2b90a63
Compare
2b90a63
to
2fa42cb
Compare
todo: tests
Description
Annotated decoder
Checklist
Quality
Maintenance
ouroboros-network
project.