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 MemoryExtensions.Split over string.Split to decrease allocations/improve perf #9600

Merged
merged 6 commits into from
Jan 23, 2025

Conversation

h3xds1nz
Copy link
Contributor

@h3xds1nz h3xds1nz commented Aug 18, 2024

Description

Picks off most places where string.Split is used just for parts count or where additional operations are performed on the split strings so it makes sense to work with ReadOnlySpan<char>/SpanSplitEnumerator<char> instead.

Using string.Split vs SpanSplitEnumerator<char> for counting

Method updatedFilter Mean [ns] Error [ns] StdDev [ns] Gen0 Code Size [B] Allocated [B]
Original Image(...))|*.* [68] 49.09 ns 0.676 ns 0.600 ns 0.0172 672 B 288 B
PR__EDIT Image(...))|*.* [68] 17.23 ns 0.075 ns 0.066 ns - 412 B -
Benchmark code
[Benchmark]
[Arguments("Image Files(*.BMP;*.JPG;*.GIF)|*.BMP;*.JPG;*.GIF|All files (*.*)|*.*")]
public  int Original(string updatedFilter)
{
    string[] formats = updatedFilter.Split('|');

    return formats.Length; 
}

[Benchmark]
[Arguments("Image Files(*.BMP;*.JPG;*.GIF)|*.BMP;*.JPG;*.GIF|All files (*.*)|*.*")]
public int PR__EDIT(string updatedFilter)
{
    int formatsCount = 0;
    foreach (Range range in updatedFilter.AsSpan().Split('|'))
        formatsCount++;

    return formatsCount;
}

Customer Impact

Improved performance, decreased allocations.

Regression

No.

Testing

Local build/CI.

Risk

Low.

Microsoft Reviewers: Open in CodeFlow

@h3xds1nz h3xds1nz requested review from a team as code owners August 18, 2024 20:18
@dotnet-policy-service dotnet-policy-service bot added PR metadata: Label to tag PRs, to facilitate with triage Community Contribution A label for all community Contributions labels Aug 18, 2024
harshit7962
harshit7962 previously approved these changes Jan 23, 2025
Copy link
Member

@harshit7962 harshit7962 left a comment

Choose a reason for hiding this comment

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

LGTM.

@h3xds1nz, Found the additional assignments of Range[] a very interesting way to discard or deal with unwanted and extra splits. Enjoyed reviewing this!

Can you please resolve conflicts and this is good to be taken in.

@h3xds1nz
Copy link
Contributor Author

@harshit7962 Thank you, glad you've enjoyed it! Resolved the conflicts :)

Copy link
Member

@harshit7962 harshit7962 left a comment

Choose a reason for hiding this comment

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

Re-Approving

@harshit7962 harshit7962 merged commit d6ae821 into dotnet:main Jan 23, 2025
8 checks passed
@harshit7962
Copy link
Member

@h3xds1nz Thanks again for the contribution!

@h3xds1nz
Copy link
Contributor Author

@harshit7962 Thanks again for reviewing :)

@h3xds1nz
Copy link
Contributor Author

@harshit7962 you didn't squash by the way, all my ugly commit messages out in the main branch now haha.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Contribution A label for all community Contributions PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants