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

Minor improvement to integration test depth prefix calculation #430

Merged
merged 1 commit into from
Sep 2, 2020

Conversation

joncloud
Copy link
Contributor

@joncloud joncloud commented Sep 2, 2020

This a really simple improvement to a calculation to create the depth prefix for logging. I was exploring the codebase, and this stood out to me. I don't think that this will have a significant improvement, but so long as the depth prefix is a single character I think it will add some benefit.

I created some simple BenchmarkDotNet tests to show a comparison between the two, and based the depth values on some that were caught while debugging the test.

Benchmark

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.Collections.Generic;
using System.Linq;

class Program
{
    static void Main(string[] args)
    {
        BenchmarkRunner.Run<Tests>();
    }
}

[MemoryDiagnoser]
public class Tests
{
    [Params(0, 1, 2, 4, 8)]
    public int Depth { get; set; }

    [Benchmark(Baseline = true)]
    public string StringConcatEnumerableRepeat() =>
        string.Concat(Enumerable.Repeat(" ", Depth));

    [Benchmark]
    public string NewString() =>
        new string(' ', Depth);
}

Results

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19041.450 (2004/?/20H1)
Intel Core i5-4258U CPU 2.40GHz (Haswell), 1 CPU, 4 logical and 2 physical cores
.NET Core SDK=5.0.100-preview.5.20279.10
  [Host]     : .NET Core 3.1.7 (CoreCLR 4.700.20.36602, CoreFX 4.700.20.37001), X64 RyuJIT
  DefaultJob : .NET Core 3.1.7 (CoreCLR 4.700.20.36602, CoreFX 4.700.20.37001), X64 RyuJIT

Method Depth Mean Error StdDev Ratio Gen 0 Gen 1 Gen 2 Allocated
StringConcatEnumerableRepeat 0 16.710 ns 0.0743 ns 0.0695 ns 1.00 - - - -
NewString 0 2.660 ns 0.0484 ns 0.0429 ns 0.16 - - - -
StringConcatEnumerableRepeat 1 42.820 ns 0.3366 ns 0.2984 ns 1.00 0.0255 - - 40 B
NewString 1 9.185 ns 0.0985 ns 0.0921 ns 0.21 0.0153 - - 24 B
StringConcatEnumerableRepeat 2 88.547 ns 1.7138 ns 1.4311 ns 1.00 0.0457 - - 72 B
NewString 2 9.807 ns 0.0920 ns 0.0816 ns 0.11 0.0204 - - 32 B
StringConcatEnumerableRepeat 4 103.794 ns 0.5806 ns 0.5431 ns 1.00 0.0457 - - 72 B
NewString 4 10.290 ns 0.1617 ns 0.1350 ns 0.10 0.0204 - - 32 B
StringConcatEnumerableRepeat 8 145.738 ns 0.6164 ns 0.5766 ns 1.00 0.0508 - - 80 B
NewString 8 12.237 ns 0.0952 ns 0.0795 ns 0.08 0.0255 - - 40 B

* Replaces LINQ with new string
* Removes unneeded namespace import
@codecov-commenter
Copy link

codecov-commenter commented Sep 2, 2020

Codecov Report

Merging #430 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #430   +/-   ##
=======================================
  Coverage   91.93%   91.93%           
=======================================
  Files         199      199           
  Lines        6708     6708           
=======================================
  Hits         6167     6167           
  Misses        541      541           
Flag Coverage Δ
#dotnet 91.93% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Bicep.Core.IntegrationTests/ParserTests.cs 98.24% <100.00%> (ø)

Copy link
Member

@majastrz majastrz left a comment

Choose a reason for hiding this comment

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

Thanks for sharing the benchmark results and for contributing!

@majastrz majastrz merged commit 381a832 into Azure:master Sep 2, 2020
@majastrz
Copy link
Member

majastrz commented Sep 2, 2020

Just out of curiosity, have you observed any noticeable improvement in end-to-end test execution time after this change?

@joncloud
Copy link
Contributor Author

joncloud commented Sep 2, 2020

I ran some pretty basic tests against the method (in both branches) with Visual Studio, and the results showed minor improvement (~20ms). I don't know that I really noticed much of a difference from a user perspective though.

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

Successfully merging this pull request may close these issues.

None yet

3 participants