Remove p/invoke to RtlMoveMemory, cleanup caller #42749
Conversation
|
Tagging subscribers to this area: @safern, @tannergooding, @jeffhandley |
|
This does not require backporting to 5.0. It's just to make the tools happy for the future. :) |
|
Just wondering, any perf numbers showing increase/decrease? |
|
|
||
| offsetSrc += sourceData.Stride; | ||
| offsetDest += targetData.Stride; | ||
| Buffer.MemoryCopy(srcPtr, destPtr, bytesToCopyEachIter, bytesToCopyEachIter); |
There was a problem hiding this comment.
Why Buffer.MemoryCopy rather than Span.CopyTo?
Also, why copy by row rather than just the entire bitmap at once (which I would imagine has higher throughput)?
There was a problem hiding this comment.
Buffer.MemoryCopy because I'm already dealing with raw pointers, so may as well use the pointer-based APIs. Converting them to span would introduce artificial complexity for no benefit. It doesn't make the code "safe", for instance.
Copy by row because the source and the destination might have different stride lengths.
There was a problem hiding this comment.
In theory, if the destination stride length is shorter than the source stride length, the copy will still "succeed". Though subsequent row copies might overwrite previous row copies, and I don't know what happens to the data beyond the last row of the dest. Hence some of my nervousness around trying to infer what an appropriate "actual dest length" parameter would be, so I'm continuing to say "eh, just assume the dest is large enough" - which matches what the original code did. I'm not trying to fix any bugs that might exist in this code, just bring the code into compliance.
There was a problem hiding this comment.
Copy by row because the source and the destination might have different stride lengths.
Ah, I didn't realize that this supported copying a subset of the original bitmap; that makes sense.
|
I don't have perf numbers for this change. This was a code cleanup / compliance change, not a perf change. Though my gut tells me there will be increased throughput since we're no longer relying on the p/invoke marshaler every iteration of the loop. |
ghost
locked as resolved and limited conversation to collaborators
Two code cleanup changes:
Remove call to
RtlMoveMemory, which is getting flagged by Semmle as prohibited. I think this usage is fine, but easier to remove the call site than to fight the tool. The p/invoke signature is incorrect anyway.Cleanup call site to use raw pointers and to call the existing framework-provided
MemoryCopyroutine. Note:MemoryCopytakes src before dest in the arguments list.