add optimized FastByteCopy #853
Conversation
|
Hi @geoffkizer, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
|
Vectors are better > 🚀 |
|
Yeah, Vectors are better, at least in theory. Unsafe library would be needed to make this work well, I think. That said, vector size can vary based on hardware, so that would make the code a bit more involved; not sure it's worth it for these scenarios. BTW, I played around with this a bit, but it's certainly worth looking at more. There may be tweaks we could do to make this better. Ideally we could have the JIT do smart things here, e.g. do appropriate vector optimization, generate fixed size copies, etc. That's probably a longer term thing. Largest copy in TechEmpower is 37 bytes, for the Date header. |
|
Actually do have a change can drop one of the fixed blocks in all cases |
| @@ -687,7 +790,7 @@ public MemoryPoolIterator CopyTo(byte[] array, int offset, int count, out int ac | |||
| actual = count; | |||
| if (array != null) | |||
| { | |||
| Buffer.BlockCopy(block.Array, index, array, offset, remaining); | |||
| FastByteCopy(block.Array, (uint)index, array, (uint)offset, (uint)remaining); | |||
There was a problem hiding this comment.
can use block.DataFixedPtr rather than fixed on block.Array
|
I think fixed() here is basically a no-op. |
|
|
||
| if (byteCount < 64) | ||
| { | ||
| // Buffer.BlockCopy is expensive for small copies. Do it manually ourselves. |
There was a problem hiding this comment.
Why doesn't Buffer.BlockCopy do this?
There was a problem hiding this comment.
Buffer.BlockCopy has to do type checks regardless, so it's better to have an unsafe version that doesn't require this.
Perhaps we should have something like Buffer.UnsafeByteCopy, but that's a larger discussion.
There was a problem hiding this comment.
I mean, we have other code like this in kestrel but it's not great that we need to do it.
There was a problem hiding this comment.
I agree, I'm just saying that figuring out a better way is probably a longer discussion. Do you want to have that discussion before we decide whether to take this?
There was a problem hiding this comment.
|
@geoffkizer this any good? benaadams@201139a?w=1 |
|
Actually this is probably better benaadams@757f7e4?w=1 |
|
I'm going to merge this as is. @davidfowl Sound good to you? |
|
@geoffkizer For small byte copies do you think the cost of |
|
I tried using DataFixedPtr and it didn't seem to matter. |
|
The Debug.Asserts are still testing the wrong things; they are testing against the entire slab array size rather than the individual block size |
|
As it stands, FastByteCopy is intentionally ignorant of Blocks, so that it can be a general purpose copy mechanism. That's why the asserts are the way they are. |
|
Which is why I added the asserts to CopyTo at the block level so it would assert if any of the copies were going outside of the block. As the block is a small window over a much larger array the asserts in the FastByteCopy are unlikely to ever trigger even if CopyTo is being asked to inadvertently overwrite adjacent blocks; as is far as its concerned its a valid request. |
|
Sorry, they are additional asserts that weren't there previously and the behaviour is not changing on BlockCopy, so out of scope will raise other PR. |
|
Added PR #900 for other asserts |
|
@geoffkizer @davidfowl Closing since we want #897 for 1.0.1, and we didn't think this kind of change was worth merging temporarily for 1.0.0. |
Buffer.BlockCopy is not particularly efficient for small blocks.
For blocks < 64 bytes, do the copy ourselves in unsafe code.
This gives something like 4% on TechEmpower plaintext (when combined with other proposed fixes).