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

String optimizations #122

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

String optimizations #122

wants to merge 1 commit into from

Conversation

jnyrup
Copy link
Contributor

@jnyrup jnyrup commented Dec 19, 2017

  • Instead of calling ToLower[Invariant] on a string, we can use a StringComparison overload of Equals or Compare.
  • Instead of calling Substring on a string, we can use String.Compare[Ordinal] to perform equality checks on parts of a string.
  • Concatenating a primitive without calling ToString() will box it in an object on which ToString() is then called.
  • Instead of concatenating strings and then appending the concatenated string to a StringBuilder we can avoid some allocations by appending each string to the StringBuilder instead.
  • Delayed some invocations of e.g. ÌsHtml as they were only used inside an if.

@JonathanMagnan JonathanMagnan self-assigned this Dec 19, 2017
@JonathanMagnan
Copy link
Member

Hello @jnyrup ,

Thank you for your contribution,

We will try to review all your changes this week.

Best Regards,

Jonathan


Help us to support this library: Donate

@JonathanMagnan
Copy link
Member

Hello @jnyrup ,

Do you have any information about this one: "Concatenating a primitive without calling ToString() will box it in an object on which ToString() is then called." ?

I believe, ToString is implicitly called. Even in my C# Expression Evaluator, when a concatenate string with primitive, I implicitly call ToString()

ReSharper also marks this code as Redundant.

Best Regards,

Jonathan


Help us to support this library: Donate

@jnyrup
Copy link
Contributor Author

jnyrup commented Dec 22, 2017

Hi @JonathanMagnan

According to https://dotnetfiddle.net/s1jy96

using System;
					
public class Program
{
	public static void Main()
	{
		Console.WriteLine(Boxing(1));
		Console.WriteLine(NoBoxing(1));
	}
	
	private static string Boxing(int i){
		return "a" + i;
	}
	
	private static string NoBoxing(int i){
		return "b" + i.ToString();
	}
}

generates the following IL

.method private hidebysig static string 
          Boxing(int32 i) cil managed
  {
    // 
    .maxstack  2
    .locals init (string V_0)
    IL_0000:  nop
    IL_0001:  ldstr      "a"
    IL_0006:  ldarg.0
    IL_0007:  box        [mscorlib]System.Int32
    IL_000c:  call       string [mscorlib]System.String::Concat(object,
                                                                object)
    IL_0011:  stloc.0
    IL_0012:  br.s       IL_0014

    IL_0014:  ldloc.0
    IL_0015:  ret
  } // end of method Program::Boxing

  .method private hidebysig static string 
          NoBoxing(int32 i) cil managed
  {
    // 
    .maxstack  2
    .locals init (string V_0)
    IL_0000:  nop
    IL_0001:  ldstr      "b"
    IL_0006:  ldarga.s   i
    IL_0008:  call       instance string [mscorlib]System.Int32::ToString()
    IL_000d:  call       string [mscorlib]System.String::Concat(string,
                                                                string)
    IL_0012:  stloc.0
    IL_0013:  br.s       IL_0015

    IL_0015:  ldloc.0
    IL_0016:  ret
  } // end of method Program::NoBoxing

Notice the box instruction in Boxing before calling Concat(object arg0, object arg1) which calls ToString() on both its arguments and then calls Concat(string str0, str1).

In NoBoxing when calling Int32.ToString() first, we call directly into Concat(string str0, str1).

@zgabi
Copy link
Contributor

zgabi commented Dec 22, 2017

Thats true currently, but my opinion is that it will be an "ugly hack" in HAP's source code, since it should be a compiler feature, see: dotnet/roslyn#10966

@JonathanMagnan
Copy link
Member

Thank you @jnyrup ,

I believe also we can skip this one. Not sure what's the faster between boxing and calling ToString but I don't think it really matters.

Thank you @zgabi for the additional information.

I will try to include other changes in the next release.

Best Regards,

Jonathan

@jnyrup
Copy link
Contributor Author

jnyrup commented Dec 24, 2017

ToString is called in either case -- at some point the int needs to be converted into a string representation.
If not called explicitly, it is done inside Concat(object, object).
So calling ToString explicitly should be faster.
I haven't performed any benchmarks to see if it's a measurable faster though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants