Accurately count only newly examined bytes#12639
Conversation
|
TODO: Write regression tests. I just wanted everyone to see the proposed preview8 changes ASAP. |
- Ensure Kestrel count all bytes read using TryRead - Ensure Kestrel doesn't double count examined but not consumed bytes
fe87720 to
f53ebed
Compare
|
@anurse @davidfowl I think we should take this for preview8. |
|
Any potential performance regression here? |
|
Doubt it |
Agreed, but just from a bureaucracy perspective, if it doesn't happen I feel confident taking this in preview 9. It seems edge-casey enough that I don't know if I'd block preview8 for this fix, but read-rate accounting is critical to the product so getting it in for preview9 seems fair. Anyway, that's all moot if we can land it by Monday. Just setting the context and contingency plans. |
|
We will do cleanup for preview9. Functionally looks sound to me. |
| { | ||
| _context.TimeoutControl.BytesRead(bytesRead - _alreadyTimedBytes); | ||
| _alreadyTimedBytes = 0; | ||
| var numFirstSeenBytes = bytesInReadResult - _alreadyTimedBytes; |
There was a problem hiding this comment.
@halter73 Should this then update _alreadyTimedBytes?
var numFirstSeenBytes = bytesInReadResult - _alreadyTimedBytes;
if (numFirstSeenBytes > 0)
{
_context.TimeoutControl.BytesRead(numFirstSeenBytes);
_alreadyTimedBytes = bytesInReadResult ;
}There was a problem hiding this comment.
Yeah. We count all the bytes in the ReadResult towards rate calculations, examined or not. Then in Advance, if we don't consume everything, we count all the unconsumed and unexamined bytes towards _alreadyTimedBytes, since both the unconsumed and unexamined bytes will be present in the next read result. What got you looking at this?
There was a problem hiding this comment.
Was getting test failures with #11942 (comment) when I removed all the changes that weren't directly related to the streams; as assumed this superseded them.
Issue wasn't with counting though; needed TryStart/TryStop in Http2MessageBody
not consumed bytes
See #11942 (comment) for context