[API Proposal]: Analyzer - replace copy loop with span.CopyTo(destination)

[neon-sunset]

Background and motivation

Given copy unrolling improvements, using span.CopyTo(dest) should outperform manual copy with a loop in almost every situation. It may be desirable to have a performance analyzer rule to flag such use cases and suggest an auto-fixer to address them.

Example pattern spotted in

runtime/src/libraries/System.Private.CoreLib/src/System/IO/StreamWriter.cs

Lines 405 to 413 in 2ab51c6

	if (buffer.Length <= 4 && // Threshold of 4 chosen based on perf experimentation
	buffer.Length <= _charLen - _charPos)
	{
	// For very short buffers and when we don't need to worry about running out of space
	// in the char buffer, just copy the chars individually.
	for (int i = 0; i < buffer.Length; i++)
	{
	_charBuffer[_charPos++] = buffer[i];
	}

API Usage

var src = Enumerable
    .Range(0, 16)
    .Select(num => num.ToString())
    .ToArray()
    .AsSpan()
var dst = new string[16];

// Flagged with a rule "use span.CopyTo for copying elements"
for (var i = 0; i < src.Length; i++)
{
    dst[i] = src[i];
}

Alternative Designs

N/A

Risks

Should be none (right? :) )

[EgorBo]

The only potential problem here that CopyTo relaxes atomicity guarantees, so you wrote a code that atomically (per our memory model) writes/copies 2-byte elements, now you're suggesting to replace it with CopyTo that might produce torn values in a multi-thread environment.

This problem is one of the reason we don't invest a lot into loop autovectorization in JIT

[neon-sunset]

This could possibly have heuristics like copying elements below or equal to pointer size will appear with greater or the same degree of atomicity. So no kicking in for structs that could be torn - it will cover most common case like assigning object references, integers, etc. (yes, 32bit systems, but there are tons of bespoke copy loops for integers, chars as linked, and more)

[MichalPetryka]

This could possibly have heuristics like copying elements below or equal to pointer size will appear with greater or the same degree of atomicity. So no kicking in for structs that could be torn - it will cover most common case like assigning object references, integers, etc. (yes, 32bit systems, but there are tons of bespoke copy loops for integers, chars as linked, and more)

CopyTo doesn't provide any atomicity guarantees, on ARM64 it should be atomic for pointer accesses at pointer aligned locations but on X64 it's not guaranteed.

[neon-sunset]

CopyTo doesn't provide any atomicity guarantees, on ARM64 it should be atomic for pointer accesses at pointer aligned locations but on X64 it's not guaranteed.

Right, and spans don't provide alignment guarantees for these either (if they did, then there would be atomicity guarantees).

So, the only element types left are byte and object.

[huoyaoyuan]

I'm now curious that: does Span.CopyTo guarantee proper element-wise access for properly aligned spans?

Tagging subscribers to this area: @dotnet/area-system-memory
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and motivation

Given copy unrolling improvements, using span.CopyTo(dest) should outperform manual copy with a loop in almost every situation. It may be desirable to have a performance analyzer rule to flag such use cases and suggest an auto-fixer to address them.

Example pattern spotted in

runtime/src/libraries/System.Private.CoreLib/src/System/IO/StreamWriter.cs

Lines 405 to 413 in 2ab51c6

	if (buffer.Length <= 4 && // Threshold of 4 chosen based on perf experimentation
	buffer.Length <= _charLen - _charPos)
	{
	// For very short buffers and when we don't need to worry about running out of space
	// in the char buffer, just copy the chars individually.
	for (int i = 0; i < buffer.Length; i++)
	{
	_charBuffer[_charPos++] = buffer[i];
	}

API Usage

var src = Enumerable
    .Range(0, 16)
    .Select(num => num.ToString())
    .ToArray()
    .AsSpan()
var dst = new string[16];

// Flagged with a rule "use span.CopyTo for copying elements"
for (var i = 0; i < src.Length; i++)
{
    dst[i] = src[i];
}

Alternative Designs

N/A

Risks

Should be none (right? :) )

Author:	neon-sunset
Assignees:	-
Labels:	api-suggestion, area-System.Memory, untriaged, code-analyzer
Milestone:	-

[EgorBo]

I'm now curious that: does Span.CopyTo guarantee proper element-wise access for properly aligned spans?

With the current implementation it does not

[EgorBo]

So overall suggestions like these are probably nice to have! (poor man's loop-auto-vectorization) but the user should be aware of risks

[neon-sunset]

Thanks for the feedback.

Perhaps this could be a warning for loops where the analyzer sees that atomicity is preserved (bytes and objects) while for other cases it might be a suggestion, further relying on whether the buffer is declared locally and is not observed before the copy is done?

This seems like a fairly widespread mistake to copy the elements manually, and if the users case about atomicity, it is likely they already know which option to go with.