Use Existing Method or Rewrite - Helper Library

Question

I have a class library that is full of typical methods that I use a lot, especially in my domain. Most of this library has remained unchanged for years at this point. More adds than changes.

In the library there's a lot of calling methods within the library - using itself.

From a complete optimization point of view - because this lib is used a lot what makes more sense:

Use methods within the lib or re-write the code within every method to avoid calling into another method. What's the cost of calling another method or maybe two methods deep versus having the code in the called method.

For instance, this illustrates a common scenario. This method turns what's typically an ugly URL parameter (with encoded html) into a more simple, hackable, date with dashes. So it gets called hundreds of times maybe thousands on a page by hundreds of users (so maybe not a trivial amount of times?).

The reason I don't think this is pre-optimization or micro optimization (hence why I'm asking) is that since this is a library and is being used by many applications, on the same server, with 100s of users, the "micro" savings could really add up.

string GeturlDate(this DateTime date)
{
   return date.GetUrlDate(date, "-");
}
string GetUrlDate(this DateTime date, string delimiter)
{
   return DateHelper.GetUrlDate(date, delimiter);
}

string DateHelper.GetUrlDate(DateTime date, string delimiter)
{
   return string.format("{0}-{1}-{2}", date...);
}

In this case, the final method with string.format could be done directly in each of the methods. Avoiding the top most method going two methods in. The first two are extension methods and the last one is a direct call.

Let's skip the overload options (that's used). And while the above is undoubtedly better for maintenance - the last piece of code is in one place, how much more efficient would it be. Does the IL already inline it cause it knows? Is that all handled by the compiler and is not really jumping through methods like one might think?

For complicated functions that could easily introduce errors I would keep one place.

EDIT: For clarification around the concept of micro-optimizations and why I think thinking about these things is valid.

• Many times you do not know you have a performance problem - that doesn't mean there isn't one - or one that only rears its head under certain conditions
• There are cases where micro-optimization is used too quickly for "this is the way I've always done it and it works (& there's no perf issues)" instead of considering: "is there a better way to do this" (perhaps under certain conditions)
• Small wins matter for hot paths. A change that results in a 100ns improvement, or less memory consumption, by a factor of 1K or 100K or 1M can result in a perceptible difference.

I think if knowing that writing code a certain way can lead to better "x" then it's a valid consideration and perhaps implementation (especially if you're doing it the first time and not a refactor).

Ultimately, the answers to this question show that amongst other things, what do compiler does and how the BCL itself is written, there's no reason for a change.

Which is good news :)

Answer 1

After I fixed your code, I tried using VS with Release mode debugging enabled and Goto Disassembly after hitting a breakpoint, using [MethodImpl(MethodImplOptions.AggressiveInlining)] and [MethodImpl(MethodImplOptions.NoInlining)] and no hints to the compiler.

With AggressiveInlining, it appeared the JIT inlined all the calls from GetUrlDate() to DateHelper.GetUrlDate(DateTime date, string delimiter) into the Main method. With no MethodImplOptions set, Main method appeared to call DateHelper.GetUrlDate directly. With NoInlining, every method was called, which is also what the IL shows.

However, timing shows that switching between Release AggressiveInlining to Debug NoInlining is only an increase of 46 nanoseconds per call, which seems awfully small to worry about hand-inlining your code.

Answer 2

Have you A) identified an actual performance problem that B) can be attributed to these particular methods, specifically the C) overloads/wrappers? If not, this seems like guessing at a solution for what you're guessing may be a problem.

Also, take a look at, say, the string.Format() method overloads themselves; it stands to reason performance would be critical for those since they're used everywhere throughout .NET, and yet they all wrap a call to FormatHelper() instead of having duplicated implementations.

With that in mind you could remove one layer of methods calls by changing this...

public static class DateExtensions
{
    public static string GetUrlDate(this DateTime date)
    {
        return date.GetUrlDate("-");
    }

    public static string GetUrlDate(this DateTime date, string delimiter)
    {
        return DateHelper.GetUrlDate(date, delimiter);
    }
}

...to this...

public static class DateExtensions
{
    public static string GetUrlDate(this DateTime date)
    {
        return DateHelper.GetUrlDate(date, "-");
    }

    public static string GetUrlDate(this DateTime date, string delimiter)
    {
        return DateHelper.GetUrlDate(date, delimiter);
    }
}

...so instead of one overload calling another they both are calling DateHelper.GetUrlDate(DateTime, String) directly. Before you go unwrapping your wrapper methods, though, consider this BenchmarkDotNet benchmark...

using System;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Jobs;

namespace SO59976711
{
    public static class DateExtensions
    {
        public static string GetUrlDate(this DateTime date)
        {
            return date.GetUrlDate("-");
        }

        public static string GetUrlDate(this DateTime date, string delimiter)
        {
            return DateHelper.GetUrlDate(date, delimiter);
        }
    }

    public static class DateHelper
    {
        public static string GetUrlDate(DateTime date, string delimiter)
        {
            return string.Format("{0:yyyy}{1}{0:MM}{1}{0:dd}", date, delimiter);
        }
    }

    [SimpleJob(RuntimeMoniker.Net48)]
    [SimpleJob(RuntimeMoniker.NetCoreApp31)]
    public class DateFormattingBenchmarks
    {
        private static readonly DateTime TestDate = DateTime.Today;
        private const string TestDelimiter = "-";

        [Benchmark(Baseline = true)]
        public string String_Format()
        {
            // Use the same implementation as DateHelper.GetUrlDate() as a baseline
            return string.Format("{0:yyyy}{1}{0:MM}{1}{0:dd}", TestDate, TestDelimiter);
        }

        [Benchmark()]
        public string DateExtensions_GetUrlDate_DefaultDelimiter()
        {
            return TestDate.GetUrlDate();
        }

        [Benchmark()]
        public string DateExtensions_GetUrlDate_CustomDelimiter()
        {
            return TestDate.GetUrlDate(TestDelimiter);
        }

        [Benchmark()]
        public string DateHelper_GetUrlDate()
        {
            return DateHelper.GetUrlDate(TestDate, TestDelimiter);
        }
    }

    public static class Program
    {
        public static void Main()
        {
            BenchmarkDotNet.Running.BenchmarkRunner.Run<DateFormattingBenchmarks>();
        }
    }
}

...which yields these results...

// * Summary *

BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i7 CPU 860 2.80GHz (Nehalem), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=3.1.101
  [Host]     : .NET Core 3.1.1 (CoreCLR 4.700.19.60701, CoreFX 4.700.19.60801), X64 RyuJIT
  Job-RTUGNF : .NET Framework 4.8 (4.8.4075.0), X64 RyuJIT
  Job-NPEBBX : .NET Core 3.1.1 (CoreCLR 4.700.19.60701, CoreFX 4.700.19.60801), X64 RyuJIT


|                                     Method |       Runtime |       Mean |   Error |  StdDev | Ratio |
|------------------------------------------- |-------------- |-----------:|--------:|--------:|------:|
|                              String_Format |      .NET 4.8 | 1,044.6 ns | 6.71 ns | 5.60 ns |  1.00 |
| DateExtensions_GetUrlDate_DefaultDelimiter |      .NET 4.8 | 1,040.0 ns | 4.55 ns | 4.26 ns |  1.00 |
|  DateExtensions_GetUrlDate_CustomDelimiter |      .NET 4.8 | 1,045.6 ns | 8.31 ns | 6.49 ns |  1.00 |
|                      DateHelper_GetUrlDate |      .NET 4.8 | 1,045.0 ns | 6.18 ns | 5.47 ns |  1.00 |
|                                            |               |            |         |         |       |
|                              String_Format | .NET Core 3.1 |   623.7 ns | 4.92 ns | 4.36 ns |  1.00 |
| DateExtensions_GetUrlDate_DefaultDelimiter | .NET Core 3.1 |   624.9 ns | 2.89 ns | 2.71 ns |  1.00 |
|  DateExtensions_GetUrlDate_CustomDelimiter | .NET Core 3.1 |   618.5 ns | 2.48 ns | 2.07 ns |  0.99 |
|                      DateHelper_GetUrlDate | .NET Core 3.1 |   621.4 ns | 2.97 ns | 2.48 ns |  1.00 |

As you can see, there is no performance difference between calling any of your three helper methods and string.Format() itself. This means you should implement your methods in whatever way is the most maintainable with the least redundancy (i.e. keep them the way they are) because there are no gains to be had by duplicating code or "pre-inlining" inlinable methods.

Answer 3

IMO - forgive the impertinence - this actually is a perfect example of unnecessary micro-optimization. If you are not actually seeing a performance problem, and you are writing sensible code (which you are), then the case for optimization is weak. Nesting of calls is a truly negligible performance consideration, and for simple functions like this you should not waste your time thinking about IL!. You should benchmark the code to make a case for it if you truly think there's a performance problem. I think you'll find you're wasting brain cycles.