Avoid unnecessary work for identical locations in Buffer.BlockCopy #36

Merged
merged 1 commit into from Feb 4, 2015

Projects

None yet

3 participants

@benaadams
Contributor

Avoid unnecessary work for identical locations in Buffer.BlockCopy

Perform validity checks to ensure parameters are correct but short-circuit
out memmove when exactly the same data would be copied to the same location.

There are a number of occasions; which can be intentional or unintentional,
where the buffer being copied is the same place - e.g an internal buffer is
the same as the return buffer, and there is no need to call memmove's
overwrite safe copy.

Generally the call to BlockCopy will be in a library so it is more practical
to enable the check here rather than alter all the calling functions,
including 3rd party libraries to preform additional checks.

@AlexGhiondea
Member

Could you also update the commit message to follow the published guidelines?

https://github.com/dotnet/corefx/wiki/Contributing#commits

@benaadams benaadams commented on the diff Feb 4, 2015
src/vm/comutilnative.cpp
- if (count > 0) {
- memmove(dst->GetDataPtr() + dstOffset, src->GetDataPtr() + srcOffset, count);
+ if ((srcPtr != dstPtr) && (count > 0)) {
+ memmove(dstPtr, srcPtr, count);
}
FC_GC_POLL();
@benaadams
benaadams Feb 4, 2015 Contributor

Should FC_GC_POLL() also be moved inside the if test, i.e. only run when work is done?

@jkotas
jkotas Feb 4, 2015 Member

FC_GC_POLL should be done all the time even when no work is done. Otherwise, there is a risk of GC starvation - the suspension for GC would take hang if there is a thread that calls BlockCopy in a tight loop.

@jkotas
Member
jkotas commented Feb 4, 2015

Could you please update the commit message to something like "Avoid unnecessary work for identical locations in Buffer.BlockCopy" so that it follows the guidelines that Alex mentioned above?

@benaadams benaadams changed the title from BlockCopy: Zero copy for identical locations to Avoid unnecessary work for identical locations in Buffer.BlockCopy Feb 4, 2015
@benaadams
Contributor

Done

@jkotas
Member
jkotas commented Feb 4, 2015

LGTM

@AlexGhiondea
Member

LGTM -- would you mind squashing the commits? Then we are good to go!

@benaadams @benaadams benaadams Avoid unnecessary work for identical locations in Buffer.BlockCopy
Perform validity checks to ensure parameters are correct but short-circuit
out memmove when exactly the same data would be copied to the same location.

There are a number of occasions; which can be intentional or unintentional,
where the buffer being copied is the same place - e.g an internal buffer is
the same as the return buffer, and there is no need to call memmove's
overwrite safe copy.

Generally the call to BlockCopy will be in a library so it is more practical
to enable the check here rather than alter all the calling functions,
including 3rd party libraries to preform additional checks.
a5dd659
@benaadams
Contributor

Squashed

@jkotas jkotas merged commit cbf5cbf into dotnet:master Feb 4, 2015

1 check passed

default Merged build finished.
Details
@jkotas
Member
jkotas commented Feb 4, 2015

Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment