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<Char>.Fill in String(Char, Int32) constructor #57993

Closed
Tracked by #64603
jfd16 opened this issue Aug 24, 2021 · 11 comments
Closed
Tracked by #64603

Use Span<Char>.Fill in String(Char, Int32) constructor #57993

jfd16 opened this issue Aug 24, 2021 · 11 comments
Assignees
Labels
area-System.Runtime help wanted [up-for-grabs] Good issue for external contributors tenet-performance Performance related issue
Milestone

Comments

@jfd16
Copy link
Contributor

jfd16 commented Aug 24, 2021

This would most likely be a significant performance improvement, especially with the vectorized span fill introduced in #51365.

@dotnet-issue-labeler
Copy link

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

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Aug 24, 2021
@GrabYourPitchforks GrabYourPitchforks added area-System.Runtime help wanted [up-for-grabs] Good issue for external contributors and removed untriaged New issue has not been triaged by the area owner labels Aug 24, 2021
@ghost
Copy link

ghost commented Aug 24, 2021

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

Issue Details

This would most likely be a significant performance improvement, especially with the vectorized span fill introduced in #51365.

Author: jfd16
Assignees: -
Labels:

area-System.Runtime, up-for-grabs

Milestone: -

@GrabYourPitchforks
Copy link
Member

@jfd16 Thanks for the suggestion! I've marked it up for grabs so that anybody can tackle it if they're interested. Or feel free to submit a PR if you're feeling up to it.

For whoever submits a PR: please also submit benchmarks demonstrating that this is a perf win for common use cases. Maybe try lengths of 1, 2, 5, and 10 as typical use cases (to mimic code which emits padding); or 4, 8, 12, and 16 (to mimic code which emits spaces as a tab char replacement).

@ahydrax
Copy link

ahydrax commented Aug 24, 2021

@GrabYourPitchforks I can look into it. Could you assign this to me?

@jeffhandley jeffhandley added this to the Future milestone Sep 15, 2021
@SteveDunn
Copy link
Contributor

I was curious about this one, so I did the change and ran the benchmarks. The results were surprising though; it was slower!

Here's the results:

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.22000
Intel Core i7-7700K CPU 4.20GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET SDK=7.0.100-alpha.1.21467.19
  [Host]     : .NET 5.0.10 (5.0.1021.41214), X64 RyuJIT
  Job-KJFVRU : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT
  Job-MZHKWO : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT

IterationCount=3  LaunchCount=1  WarmupCount=3  
Method Job Toolchain Size Mean Error StdDev Ratio RatioSD Gen 0 Allocated
Medium Job-KJFVRU main 1 6.512 ns 2.859 ns 0.1567 ns 1.00 0.00 0.0057 24 B
Medium Job-MZHKWO PR 1 13.985 ns 2.461 ns 0.1349 ns 2.15 0.07 0.0134 56 B
Medium Job-KJFVRU main 2 6.360 ns 1.075 ns 0.0589 ns 1.00 0.00 0.0076 32 B
Medium Job-MZHKWO PR 2 14.040 ns 3.786 ns 0.2075 ns 2.21 0.03 0.0153 64 B
Medium Job-KJFVRU main 4 6.149 ns 3.932 ns 0.2155 ns 1.00 0.00 0.0076 32 B
Medium Job-MZHKWO PR 4 14.773 ns 12.972 ns 0.7110 ns 2.40 0.10 0.0153 64 B
Medium Job-KJFVRU main 5 6.680 ns 1.670 ns 0.0916 ns 1.00 0.00 0.0076 32 B
Medium Job-MZHKWO PR 5 13.978 ns 12.688 ns 0.6955 ns 2.09 0.08 0.0172 72 B
Medium Job-KJFVRU main 8 7.167 ns 3.092 ns 0.1695 ns 1.00 0.00 0.0096 40 B
Medium Job-MZHKWO PR 8 15.836 ns 3.509 ns 0.1924 ns 2.21 0.05 0.0191 80 B
Medium Job-KJFVRU main 10 7.764 ns 5.893 ns 0.3230 ns 1.00 0.00 0.0115 48 B
Medium Job-MZHKWO PR 10 15.461 ns 9.475 ns 0.5194 ns 1.99 0.07 0.0229 96 B
Medium Job-KJFVRU main 12 7.629 ns 3.200 ns 0.1754 ns 1.00 0.00 0.0115 48 B
Medium Job-MZHKWO PR 12 16.313 ns 12.362 ns 0.6776 ns 2.14 0.12 0.0229 96 B
Medium Job-KJFVRU main 16 8.485 ns 4.583 ns 0.2512 ns 1.00 0.00 0.0134 56 B
Medium Job-MZHKWO PR 16 16.567 ns 14.305 ns 0.7841 ns 1.96 0.14 0.0268 112 B
Medium Job-KJFVRU main 32 10.847 ns 2.463 ns 0.1350 ns 1.00 0.00 0.0210 88 B
Medium Job-MZHKWO PR 32 17.667 ns 6.613 ns 0.3625 ns 1.63 0.03 0.0421 176 B
Medium Job-KJFVRU main 64 16.476 ns 6.452 ns 0.3537 ns 1.00 0.00 0.0363 152 B
Medium Job-MZHKWO PR 64 24.305 ns 5.120 ns 0.2806 ns 1.48 0.02 0.0727 304 B
Medium Job-KJFVRU main 128 29.586 ns 14.826 ns 0.8127 ns 1.00 0.00 0.0669 280 B
Medium Job-MZHKWO PR 128 35.916 ns 33.364 ns 1.8288 ns 1.22 0.09 0.1339 560 B
Medium Job-KJFVRU main 256 54.535 ns 22.270 ns 1.2207 ns 1.00 0.00 0.1281 536 B
Medium Job-MZHKWO PR 256 61.755 ns 20.355 ns 1.1157 ns 1.13 0.02 0.2563 1,072 B

@jkoritzinsky
Copy link
Member

@SteveDunn if the implementation for the "PR" case is the code in your PR, I think there's some improvements we can do to make it perform much better (and possibly faster than main)

@ahydrax
Copy link

ahydrax commented Oct 12, 2021

I've been playing around finding conditions where SpanHelpers.Fill could perform better than current implementation. It looks like adding nested if on string length adds a performance regression.
I stucked with this:
https://github.com/ahydrax/runtime/blob/string-ctor-span-fill/src/libraries/System.Private.CoreLib/src/System/String.cs#L329

@ahydrax
Copy link

ahydrax commented Oct 12, 2021

My benchmark: https://github.com/ahydrax/string-ctor-span-fill

I would be thankful for any comments on that

@adamsitnik
Copy link
Member

adamsitnik commented Nov 15, 2021

Edit: Fixed by #60269

@adamsitnik adamsitnik modified the milestones: Future, 7.0.0 Nov 15, 2021
@adamsitnik adamsitnik added the tenet-performance Performance related issue label Nov 15, 2021
@GrabYourPitchforks
Copy link
Member

@adamsitnik Did you mean to include a different link in your message above?

@adamsitnik
Copy link
Member

Did you mean to include a different link in your message above?

@GrabYourPitchforks Yes, I meant #60269. Apologies for the confusion

@ghost ghost locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime help wanted [up-for-grabs] Good issue for external contributors tenet-performance Performance related issue
Projects
None yet
Development

No branches or pull requests

7 participants