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

Reduce memory overhead of TestNameGenerator #3594

Merged
merged 1 commit into from
Aug 25, 2020

Conversation

stevenaw
Copy link
Member

@stevenaw stevenaw commented Jul 13, 2020

This PR reduces memory overhead through two main improvements:

  • Reduce the number of string allocations or instances of StringBuilder by passing the top-level StringBuilder instance in TestNameGenerator down into the different NameFragments
  • Eliminate a few boxing operations inside of NameFragment.GetDisplayString by converting object arg into a primitive type (when possible) before comparing it to another primitive

This is a bit of an implementation of the approach suggested in #3498 , though these changes happen to be fully encapsulated within this particular class with no public/internal method signature changes.

Benchmarks

View Results
|             Method |        Mean |      Error |    StdDev |  Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------------- |------------:|-----------:|----------:|-------:|------:|------:|----------:|
| Original_SingleArg |    389.7 ns |   85.82 ns |   4.70 ns | 0.1626 |     - |     - |     680 B |
|      New_SingleArg |   383.29 ns | 324.930 ns | 17.811 ns | 0.0801 |     - |     - |     336 B |
|   Original_ManyArg |  1,886.9 ns |  468.58 ns |  25.68 ns | 0.7553 |     - |     - |    3161 B |
|        New_ManyArg | 1,841.49 ns | 254.842 ns | 13.969 ns | 0.5074 |     - |     - |    2129 B |
|     Original_NoArg |    144.8 ns |   81.61 ns |   4.47 ns | 0.0937 |     - |     - |     392 B |
|          New_NoArg |    90.09 ns |   7.174 ns |  0.393 ns | 0.0343 |     - |     - |     144 B |
  
View Code
  [ShortRunJob]
  [MemoryDiagnoser]
  public class TestNameGenerator_GetDisplayName
  {
      private static void NoArgs() { }
      private static T Generic1Arg<T>(T t) => t;
      private static T1 GenericManyArg<T1, T2, T3, T4, T5, T6, T7>(T1 t1, T2 t2, T3 t3, T4 t4, T5 t5, T6 t6, T7 t7) => t1;


      private static readonly TestMethod NoArgMethod = GenerateFromMethodInfo(nameof(NoArgs));
      private static readonly TestMethod SingleArgMethod = GenerateFromMethodInfo(nameof(Generic1Arg));
      private static readonly TestMethod ManyArgMethod = GenerateFromMethodInfo(nameof(GenericManyArg));

      private static readonly object[] SingleArg = new object[] { 42 };
      private static readonly object[] ManyArgs = new object[] { 42, double.PositiveInfinity, null, "yabba dabba doo", (byte)65, new CustomType(), -76205367f };

      private class CustomType { }

      private static TestMethod GenerateFromMethodInfo(string localMethodName)
      {
          var type = typeof(TestNameGenerator_GetDisplayName);
          var method = type.GetMethod(localMethodName, BindingFlags.NonPublic | BindingFlags.Static);

          var testMethod = new TestMethod(new MethodWrapper(type, method));

          return testMethod;
      }

      private static readonly TestNameGenerator _newImpl = new TestNameGenerator();
      private static readonly OriginalTestNameGenerator _originalImpl = new OriginalTestNameGenerator();

      [Benchmark]
      public string Original_SingleArg() => _originalImpl.GetDisplayName(SingleArgMethod, SingleArg);

      [Benchmark]
      public string New_SingleArg() => _newImpl.GetDisplayName(SingleArgMethod, SingleArg);


      [Benchmark]
      public string Original_ManyArg() => _originalImpl.GetDisplayName(ManyArgMethod, ManyArgs);

      [Benchmark]
      public string New_ManyArg() => _newImpl.GetDisplayName(ManyArgMethod, ManyArgs);


      [Benchmark]
      public string Original_NoArg() => _originalImpl.GetDisplayName(NoArgMethod);

      [Benchmark]
      public string New_NoArg() => _newImpl.GetDisplayName(NoArgMethod);
  }

Copy link
Member

@ChrisMaddock ChrisMaddock left a comment

Choose a reason for hiding this comment

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

I'm not familiar with the refactoring being discussed in #3498 - but this looks like a nice improvement regardless! Thanks for looking at it. 🙂

It'll need a second review before merging.

@rprouse rprouse merged commit 11ebd48 into nunit:master Aug 25, 2020
@rprouse rprouse added this to the 3.13 milestone Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants