search

davidfowl / BedrockFramework

High performance, low level networking APIs for building custom servers and clients.
MIT License
1.04k stars 152 forks source link

ProtocolReader.ReadAsync doesn't advance if read is successful #70

Closed mgravell closed 4 years ago

mgravell commented 4 years ago

The AdvanceTo is only invoked when read fails; most protocols will involve multiple messages, but the second call to ReadAsync reliably fails with:

Unhandled exception. System.InvalidOperationException: Advance must be called before calling ReadAsync

Repro is here

mgravell commented 4 years ago

nm; I think I get it now - the Advance is manual After the ReadAsync. This is very unintuitive, but... I guess it works?

mgravell commented 4 years ago

FYI, initial results - noting that SE.Redis does a lot more and is optimized for different usage to this:

ExecuteBedrock: time for 1000 ops: 76ms
ExecuteStackExchagneRedis: time for 1000 ops: 92ms
mgravell commented 4 years ago

also: got it kinda sorta working in a BDN project, but now I'm seeing:

 ---> System.InvalidOperationException: End position was not reached during enumeration.
   at System.ThrowHelper.ThrowInvalidOperationException_EndPositionNotReached()
   at System.Buffers.SequenceReader`1.IsNextSlow(ReadOnlySpan`1 next, Boolean advancePast)
   at System.Buffers.SequenceReader`1.TryReadTo(ReadOnlySequence`1& sequence, ReadOnlySpan`1 delimiter, Boolean advancePastDelimiter)
   at System.Buffers.SequenceReaderExtensions.TryReadTo[T](SequenceReader`1& sequenceReader, ReadOnlySpan`1& span, ReadOnlySpan`1 delimiter, Boolean advancePastDelimiter) in C:\Code\BedrockRespProtocol\src\BedrockRespProtocol\Lifted\SequenceReaderExtensions.cs:line 7

I'm assuming that is the bug @Drawaes was telling me about?

Drawaes commented 4 years ago

Yes the very same bug.... Might need to work around it ....let me have a look tonight

mgravell commented 4 years ago

On the API design: I think I'm in favor of the explicit Advance. This ties in well with allowing me to do a zero-copy parse that just deframes segments of the original buffer. Then we can call Advance once the deframed data has been processed. So: nothing to see here!

mgravell commented 4 years ago

@Drawaes I worked around it by changing to the super-sophisticated:

if (sequenceReader.TryReadTo(out ReadOnlySequence<byte> payloadPlusPrefix, (byte)'\r')
    && sequenceReader.TryRead(out var n) && n == '\n')

which correctly locates my CR/LF pairs without falling in a mound.

Allocations are higher than the competing code, note - I think this is probably TPL overheads:

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
SERedis 84.47 us 1.214 us 1.136 us - - - 360 B
Bedrock 77.38 us 1.053 us 0.985 us - - - 704 B
mgravell commented 4 years ago

initial allocations count based on 1000 operations:

Name Total (Allocations) Self (Allocations) Total Size (Bytes) Self Size (Bytes)

going to see what happens if I use PooledAwait on that... will also see if I can get the .NET 5 bits working

mgravell commented 4 years ago

with PooledAwait on 2 of my methods, this becomes

Name Total (Allocations) Self (Allocations) Total Size (Bytes) Self Size (Bytes)

The RedisSimpleStringMemory is my actual objects - fine with those

The AsyncTaskMethodBuilder1.AsyncStateMachineBox1 are from Name Bedrock.Framework.Protocols.ProtocolReader+<DoAsyncRead>d__131[System.__Canon]::MoveNext`

The QueueUserWorkItemCallbackDefaultContext are from Bedrock.Framework.SocketReceiver+<>c::<.ctor>b__3_0 1002

mgravell commented 4 years ago

most of the allocs should just evaporate if we can justify turning on the task amortizer bits; the ones that remain are the QueueUserWorkItemCallbackDefaultContext from SocketAwaitable.Complete - I can't see how I could bypass those since PipeScheduler.ThreadPool is hard-coded in SocketConnection; thoughts on that, @davidfowl ? does that concern you at all?

JanEggers commented 4 years ago

@mgravell found the same and created https://github.com/JanEggers/BedrockTransports/commit/4ed38b6e6692621ea6909411dba512725071a919

should i create a pr or are the more buildin solutions?

what is Task amortizer bits can you post a link i would like to try that with my mqtt bits.

davidfowl commented 4 years ago

The fix is these allocations is to do https://github.com/davidfowl/BedrockFramework/issues/19. I haven't done it yet, it's mostly copy pasta from Kestrel.

mgravell commented 4 years ago

@JanEggers see https://github.com/dotnet/runtime/issues/13633

mgravell commented 4 years ago

@JanEggers that custom scheduler looks pretty hairy re concurrency; I wonder if it would be better to simply test the state arg to see if that is a IThreadPoolWorkItem - and if so, UnsafeQueueUserWorkItem it - otherwise push it to the thread-pool; or wait for David's voodoo

JanEggers commented 4 years ago

I guess voodoo is the best option

davidfowl commented 4 years ago

Closing this in favor of #19, which should solve most allocations except the state machine. That one will require more work see https://github.com/davidfowl/BedrockFramework/issues/20

mgravell commented 4 years ago

@davidfowl the state machine evaporates if we https://github.com/dotnet/runtime/issues/13633