Use MemoryExtensions in StreamsComparer for performance #4668
Conversation
|
@stevenaw Could you review this please? |
stevenaw
left a comment
stevenaw
left a comment
There was a problem hiding this comment.
Looks great to me! Thanks for your contribution @mithileshz
stevenaw
left a comment
stevenaw
left a comment
There was a problem hiding this comment.
Thanks @mithileshz ! Sorry for the false approval earlier.
This looks really great, but I left one minor suggestion here out of abundance of caution.
|
|
||
| if (MemoryExtensions.SequenceEqual<byte>(bufferExpected, bufferActual)) | ||
| { | ||
| readByte += BUFFER_SIZE; |
There was a problem hiding this comment.
| readByte += BUFFER_SIZE; | |
| readByte += readActual; |
Sorry, I had missed this in my first pass. It's a bit of a nitpick, but I think it may be safer here to increment by readActual here. It's possible for fewer than BUFFER_SIZE bytes to be read in and 99% of the time it likely won't matter as it's simply because it's near the end of the stream. For some streams like NetworkStream however, it may matter if the reason is that there simply aren't more bytes to read from the network that moment.
There was a problem hiding this comment.
That makes sense to me. Should we apply the same change to line 85 below? https://github.com/nunit/nunit/pull/4668/files/8a0a34f1cc5d469714f099c314c56a0bae3fc44d#diff-57f5421e71da296e02ec0d48cfee1a9e89e53de267929ee779797c9d24448fbcR85
There was a problem hiding this comment.
Thanks for pointing that out @mithileshz ! Good point, I can see why you approached your PR this way then. Given the current code also advances it by BUFFER_SIZE and has been fine for 15 years then I think the PR is also great in it's current state too.
Happy to merge it as-is or to wait if you'd prefer to push another changeset. How would you like to proceed?
There was a problem hiding this comment.
I'll leave the decision up to you as the maintainer of the repository :)
If we go with the approach for changing it, I would think we need to add a test for it with the newer changes and verify it works for existing scenarios. I don't mind doing it but I would prefer to do it in a separate changeset in case the change causes a regression and would make it easy to roll back the change.
Let me know what you think
There was a problem hiding this comment.
@mithileshz I'd like to merge this as-is, it's always nice when a PR lands :)
I really like your proposed approach though, and we'd definitely appreciate a follow-up PR if you wanted to do it. 🙂
|
Thanks for your contribution in this PR @mithileshz ! EDIT: As I said above, we'd happily accept a follow-up PR too if you wanted to pursue the |
3 participants
Use MemoryExtensions to check that the expected and actual are equal.
Fixes #3829