Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Avoid string.Split allocations in FrameworkName #7288

Merged
stephentoub merged 1 commit into from
Mar 31, 2016
Merged

Avoid string.Split allocations in FrameworkName #7288

stephentoub merged 1 commit into from
Mar 31, 2016

Conversation

@jamesqo
Copy link
Contributor

@jamesqo jamesqo commented Mar 27, 2016

When parsing the input in the FrameworkName constructor, we're expecting it to look something like this:

Foo=Bar,Baz=Quux

The way we handle this is by calling input.Split(','), and then splitting each of the individual components again via component.Split('='). This is less than optimal, since calling Split makes 3 unneeded allocations:

  • one for the params array
  • one for an int[] that contains the separator indices
  • one for the returned string[]

This PR replaces such use of Split with the following snippet, eliminating these allocations:

int separatorIndex = component.IndexOf('=');
if (separatorIndex == -1 || separatorIndex != component.LastIndexOf('='))
{
    throw;
}

string key = component.Substring(0, separatorIndex);
string value = component.Substring(separatorIndex + 1);

Other changes:

  • Added tests for multiple = signs.
  • Replaced some magic strings -> nameof.
  • Replaced string.Format -> string interpolation.

Related CoreCLR PR: dotnet/coreclr#3949

cc @AlexGhiondea @JonHanna @stephentoub

@jamesqo
Copy link
Contributor Author

jamesqo commented Mar 28, 2016

BTW, I intentionally held back on optimizing out the initial allocation that called Split over the commas as I did in the CoreCLR PR; that would require more changes and need part of the method to be split up into new methods.

justinvp
justinvp reviewed Mar 28, 2016
View reviewed changes
src/System.Runtime.Extensions/src/System/Runtime/Versioning/FrameworkName.cs Outdated
Copy link
Contributor

@justinvp justinvp Mar 28, 2016

Choose a reason for hiding this comment

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

Consider using SubstringTrim from Common/src/System/StringExtensions.cs

Copy link
Member

@stephentoub stephentoub Mar 28, 2016

Choose a reason for hiding this comment

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

If it's not already in use in this assembly, I don't think it's worth the extra code being included. FrameworkNames aren't instantiated very often, are they? And if they are, are they instantiated with inputs that would cause these Trims to actually trim something? (Trim already returns the original string if there's nothing to trim.)

@stephentoub
Copy link
Member

stephentoub commented Mar 29, 2016

LGTM. Can you please squash this (or at least the first four commits)?

@jamesqo
Copy link
Contributor Author

jamesqo commented Mar 29, 2016

@stephentoub Done.

@jamesqo
Copy link
Contributor Author

jamesqo commented Mar 30, 2016

test innerloop osx release build and test
test innerloop ubuntu release build and test

@stephentoub
Copy link
Member

stephentoub commented Mar 31, 2016

Thanks, @jamesqo.

@stephentoub stephentoub merged commit 1c0df54 into dotnet:master Mar 31, 2016
@jamesqo jamesqo deleted the framework-name branch March 31, 2016 03:25
@hughbe
Copy link

hughbe commented Mar 31, 2016

Good work!
Just a quick question: I did a similar optimisation in the Version class, and @stephentoub suggested I go higher and optimise Integer parsing which I've done.
Is there an argument here too for optimising string.Split?

@stephentoub
Copy link
Member

stephentoub commented Mar 31, 2016

@hughbe, how?

@hughbe
Copy link

hughbe commented Mar 31, 2016

I took a more detailed look at the PR and the String.Split code - that's not really necessary here actually, so nevermind!

@stephentoub
Copy link
Member

stephentoub commented Mar 31, 2016

It's not a question of "necessary", but rather "possible". What optimization for String.Split did you have in mind?

@jamesqo jamesqo mentioned this pull request Apr 3, 2016
Merged
@karelz karelz modified the milestone: 1.0.0-rtm Dec 3, 2016
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@stephentoub
Merge pull request dotnet/corefx#7288 from jamesqo/framework-name
1eec873 
Avoid string.Split allocations in FrameworkName

Commit migrated from dotnet/corefx@1c0df54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.0.0-rtm

Development

Successfully merging this pull request may close these issues.

7 participants

@jamesqo @stephentoub @hughbe @justinvp @karelz @AlexGhiondea @dnfclas