BedrockFramework icon indicating copy to clipboard operation
BedrockFramework copied to clipboard

Proposal: IMessageReader<T> TryParseMessage overload with SequenceReader<byte>

Open davidfowl opened this issue 5 years ago • 2 comments

Today IMessageReader<T> looks like this:

public interface IMessageReader<TMessage>
{
    bool TryParseMessage(in ReadOnlySequence<byte> input, ref SequencePosition consumed, ref SequencePosition examined, out TMessage message);
}

This works when the caller has a ReadOnlySequence<byte> (usually from a PipeReader) and is parsing messages at that level. Most of our IMessageReader<T> implementations end up creating a SequenceReader<byte> internally over the ReadOnlySequence<byte> to do further parser.

When trying to compose these parsers internally, it usually results is translating from ReadOnlySequence<byte> -> SequenceReader<byte> which is inefficient and inconvenient. Ideally you should be able to keep the SequenceReader<byte> rather than switching back to ReadOnlySequence<byte> and vice versa.

As an example, see https://github.com/YairHalberstadt/BedrockFramework/pull/1/files

Options:

  • Use SequenceReader<byte> instead of ReadOnlySequence<byte>
  • Add TryParseMessage overload for SequenceReader
public interface IMessageReader<TMessage>
{
    bool TryParseMessage(ref SequenceReader<byte> input, out TMessage message);
    bool TryParseMessage(in ReadOnlySequence<byte> input, ref SequencePosition consumed, ref SequencePosition examined, out TMessage message);
}

cc @YairHalberstadt

davidfowl avatar Jan 28 '20 07:01 davidfowl

Option 1. Has the disadvantage of not being able to gain efficiency by removing the SequenceReader. Option 2. Has the disadvantage of requiring you to duplicate code.

I suppose one option would be to add a DIM from the overload accepting a SequenceReader using the one accepting a ReadOnlySequence. When you want to use a SequenceReader internally, you reverse the direction, and implement the one accepting a ReadOnlySequence in terms of the one accepting a SequenceReader. When performance is extremely important, you can implement both seperately.

YairHalberstadt avatar Jan 28 '20 07:01 YairHalberstadt

I don't know how possible it would be to add, if only for the message readers that are ReadOnlySequence<byte> pass-throughs. However, I have been noticing that there is some API friction in general with having to implement message readers that then return sequences, so perhaps that should be a construct all its own and not IMessageReader<ReadOnlySequence<byte>>.

mattnischan avatar Jan 30 '20 04:01 mattnischan