3

I'm sure everyone's encountered their share of developers who love the ToString() method. We've all likely seen code similar to the following:

public static bool CompareIfAnyMatchesOrHasEmpty(List<string> list1, List<string> list2) { bool result = false; foreach (string item1 in list1) { foreach (string item2 in list2) { if (item1.ToString() == item2.ToString()) { result = true; } if (item1.ToString() == "") { result = true; } } } return result; }

What I'm wondering is if the ToString() method (the empty, no formatting one) can be optimized away by the compiler? My assumption is that it does not, since it's originally defined on object. Thus I provide this second question, on if any effort to cleanup such instances would be worthwhile?

Improve this question
7
  • 10
    That code just sent shivers down my spine! Commented Aug 8, 2014 at 18:10
  • 3
    be optimized away is hardly relevant, it's too simpel an operation. This is an issue of code quality, not of performance. Commented Aug 8, 2014 at 18:12
  • 5
    You'd be better served optimizing away the developer who wrote this. Commented Aug 8, 2014 at 18:13
  • By the way, is this real code? I've got other problem besides the ToString. Like the fact that the runtime is O(M*N) because there are no breaks or return statements... Commented Aug 8, 2014 at 18:28
  • Note to all: the above is contrived code, near (but not quite) the worst of what I've seen. The question was inspired by this TheDailyWTF which brought back too many bad memories. Commented Aug 8, 2014 at 18:48

2 Answers 2

Reset to default
8

The C# compiler will not optimize this away. However, at runtime, I believe this will likely get inlined by the JIT compiler in the CLR, as string.ToString() just returns itself.

String.ToString is even declared with TargetedPatchingOptOutAttribute, which allows it to be inlined by NGEN as well when it's called from other assemblies, so it's obviously an inline target.

Improve this answer
Sign up to request clarification or add additional context in comments.

5 Comments

Then it's optimized away by a compiler.
@HenkHolterman Yes - the C# compiler won't, but the JIT will.
I'm interested in TargetedPatchingOptOutAttribute. Could someone include a "dummy's guide" to what "inlined across Native Image Generator (NGen) images" as part of this answer?
@MikeGuthrie If you use NGEN, it won't (normally) inline calls that cross assembly boundaries - otherwise, an updated mscorlib would change the behavior. This attribute allows that to be inlined anyways.
Really torn accepting answers here, so wanted to comment. Appreciate your answer - it did answer the title of the question, and I learned something new. However, I went with aquinas' answer because I think it addressed the heart of the real issue, namely in his last paragraph.
7

It certainly could be optimized away by the compiler, but they probably don't because it's trivial. Before deciding whether any optimization is worthwhile, try some tests first. Let's try it!

List<string> strings = Enumerable.Range(1, 10000000).Select(x => Guid.NewGuid().ToString()).ToList(); var sw= Stopwatch.StartNew(); foreach (var str in strings) { if (!str.ToString().Equals(str.ToString())) { throw new ApplicationException("The world is ending"); } } sw.Stop(); Console.WriteLine("Took: " + sw.Elapsed.TotalMilliseconds); sw = Stopwatch.StartNew(); foreach (var str in strings) { if (!str.Equals(str)) { throw new ApplicationException("The world is ending"); } } sw.Stop(); Console.WriteLine("Took: " + sw.Elapsed.TotalMilliseconds);

Ok, so we're in a loop with 10 million items. How long does the tostring (called twice) version take compared to the non tostring version?

Here's what I get on my machine:

Took: 261.6189 Took: 231.2615

So, yeah. I saved 30 whole milliseconds over 10 million iterations. So...yeah, I'm going to say no, not worth it. At all.

Now, should the code be changed because it's stupid? Yes. I would make the argument as, "This is unnecessary and makes me think at a glance that this is NOT a string. It takes me brain cycles to process, and serves literally no purpose. Don't do it." Don't argue from an optimization point of view.

Improve this answer

2 Comments

Always state how you ran a benchmark. Release build, outside VS? This one should also be run with the other loop first.
Why should the other loop be run first? In any case: Release build, outside VS: Took: 229.8982 Took: 187.611 I ran it with the loops switched, and the results were darn near identical. I'm curious why you think it should matter?

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.