BedrockFramework icon indicating copy to clipboard operation
BedrockFramework copied to clipboard

Improve performance of ProtocolReader

Open davidfowl opened this issue 6 years ago • 5 comments

There are a few things that can be done to improve the performance of the ProtocolReader:

  • [x] - Store the last ReadResult from the PipeReader and synchronously parse the protocol while there's still a buffer around. This reduces call to ReadAsync/Advance.
  • [ ] - ProtocolReader currently uses an async state machine in ReadAsync per call, it could implement IValueTaskSource to amortize this cost and reuse the ProtocolReader itself to avoid the per operation allocation (this is tricky and should be done last)

davidfowl avatar Dec 29 '19 04:12 davidfowl

ProtocolReader currently uses an async state machine in ReadAsync per call, it could implement IValueTaskSource to amortize this cost and reuse the ProtocolReader itself to avoid the per operation allocation (this is tricky and should be done last)

Turns out this is hard because the IValueTaskSource can't use the result type (TResult). The method is generic but the type isn't. This is more complicated and would require a cache per TResult. As a best effort, the code tries to avoid the state machine when the buffer is there.

davidfowl avatar Dec 31 '19 09:12 davidfowl

Sorry for being nosy, but this is a fascinating repo.

What about a generic version of ProtocolReader?

It looks like the common case is to read the same type repeatedly, so perhaps it would be worthwhile to optimize for that case? You would be able to use IValueTaskSource<TResult> then and gain back some perf.

Also, how difficult is implementing IValueTaskSource<TResult> now that ManualResetValueTaskSourceCore<TResult> exsists?

AlgorithmsAreCool avatar Jan 05 '20 21:01 AlgorithmsAreCool

What about a generic version of ProtocolReader?

Funny, it used to be generic then it was made non generic. Maybe it makes sense to have both.

Also, how difficult is implementing IValueTaskSource<TResult> now that ManualResetValueTaskSourceCore<TResult> exsists?

Much easier than it used to be, but still not completely trivial.

davidfowl avatar Jan 05 '20 23:01 davidfowl

One reason the generic reader was remove was to allow composing protocols like this https://github.com/davidfowl/BedrockFramework/blob/08273ead26e7989649bff8ada7bfa98d2fc95388/src/Bedrock.Framework.Experimental/Protocols/HttpClientProtocol.cs#L19. Http reads the header then reads the body and those end up being different message readers

davidfowl avatar Jan 05 '20 23:01 davidfowl

Perhaps rename the non-generic version to "MultiProtocolReader" or something?

AlgorithmsAreCool avatar Jan 06 '20 02:01 AlgorithmsAreCool