Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Span.Fill in String(Char, Int32) constructor #60269

Merged
merged 3 commits into from
Nov 15, 2021

Conversation

SteveDunn
Copy link
Contributor

@SteveDunn SteveDunn commented Oct 11, 2021

Fix for Use Span.Fill in String(Char, Int32) constructor #57993

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 11, 2021
@danmoseley danmoseley changed the title Implementation Use Span.Fill in String(Char, Int32) constructor Oct 11, 2021
@ghost
Copy link

ghost commented Oct 11, 2021

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

Issue Details

Fix for Use Span.Fill in String(Char, Int32) constructor #57993

Author: SteveDunn
Assignees: -
Labels:

area-System.Runtime, community-contribution

Milestone: -

@adamsitnik adamsitnik self-assigned this Nov 15, 2021
Co-authored-by: Jeremy Koritzinsky <jkoritzinsky@gmail.com>
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've benchmarked the initial version of this PR (it was always allocating an array), against @jkoritzinsky suggestion to use Span.Fill and @CyberAndrii suggestion to call SpanHelpers.Fill directly and what we have in main.

[Benchmark]
[Arguments(1)]
[Arguments(10)]
[Arguments(20)]
[Arguments(50)]
[Arguments(100)]
[Arguments(1000)]
public string CtorCharCount(int size) => new string('a', size);

Here are the results:

BenchmarkDotNet=v0.13.1.1616-nightly, OS=Windows 10.0.22000                        
AMD Ryzen Threadripper PRO 3945WX 12-Cores, 1 CPU, 24 logical and 12 physical cores
Method Job size Mean Ratio Allocated
CtorCharCount initial 1 19.487 ns 3.45 56 B
CtorCharCount Jeremy 1 6.976 ns 1.24 24 B
CtorCharCount Andrii 1 6.857 ns 1.21 24 B
CtorCharCount main 1 5.650 ns 1.00 24 B
CtorCharCount initial 10 16.537 ns 2.50 96 B
CtorCharCount Jeremy 10 7.931 ns 1.20 48 B
CtorCharCount Andrii 10 7.924 ns 1.20 48 B
CtorCharCount main 10 6.638 ns 1.00 48 B
CtorCharCount initial 20 16.553 ns 2.20 128 B
CtorCharCount Jeremy 20 7.923 ns 1.03 64 B
CtorCharCount Andrii 20 7.769 ns 1.03 64 B
CtorCharCount main 20 7.575 ns 1.00 64 B
CtorCharCount initial 50 22.185 ns 1.70 256 B
CtorCharCount Jeremy 50 9.777 ns 0.75 128 B
CtorCharCount Andrii 50 10.152 ns 0.78 128 B
CtorCharCount main 50 13.052 ns 1.00 128 B
CtorCharCount initial 100 28.198 ns 1.36 448 B
CtorCharCount Jeremy 100 13.447 ns 0.65 224 B
CtorCharCount Andrii 100 13.445 ns 0.65 224 B
CtorCharCount main 100 20.748 ns 1.00 224 B
CtorCharCount initial 1000 172.225 ns 0.97 4,048 B
CtorCharCount Jeremy 1000 80.894 ns 0.46 2,024 B
CtorCharCount Andrii 1000 79.703 ns 0.45 2,024 B
CtorCharCount main 1000 177.664 ns 1.00 2,024 B

For small inputs (count < 20), we can see a 1 ns regression. For larger inputs, we can see an up to 2x improvement.

@tannergooding @GrabYourPitchforks What is the typical usage of new string(char, count)? Is it the right tradeoff to be made?

@tannergooding
Copy link
Member

or small inputs (count < 20), we can see a 1 ns regression

The 3945WX has a base speed of 4.0GHz, so 1ns represents about 4 cycles. Even if < 20 is common, I think that's an acceptable tradeoff here as it represents less noise than a typical memory read can have.

@danmoseley
Copy link
Member

Thanks for this improvement @SteveDunn !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants