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

performance patch of JsonConverterHelper.cs #26448

Merged
merged 2 commits into from
Apr 12, 2022
Merged

performance patch of JsonConverterHelper.cs #26448

merged 2 commits into from
Apr 12, 2022

Conversation

PiDiBi
Copy link
Member

@PiDiBi PiDiBi commented Jan 18, 2022

as far as we use this in azure web apps and it makes us problems I'm kindly suggesting this performance update

  • removed LINQ
  • regex split is compiled singeltone now

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.22000
Intel Xeon W-2133 CPU 3.60GHz, 1 CPU, 12 logical and 6 physical cores
.NET SDK=6.0.101
[Host] : .NET 5.0.13 (5.0.1321.56516), X64 RyuJIT [AttachedDebugger]
DefaultJob : .NET 5.0.13 (5.0.1321.56516), X64 RyuJIT

Method Mean Error StdDev
OriginalSplit 985.5 ns 19.27 ns 47.63 ns
NewSplit1 478.3 ns 9.56 ns 12.44 ns

On Dec 11, 2021, at 10:09 AM, Feng Yuan fyuan@microsoft.com wrote:
Found a super expensive regular expression stack in NetworkWatcherTrafficAnalytics-Prod:
image
GetPropertyName is costing 4.1 million CPU samples, 14.84% of total CPU samples in the stream I downloaded.

testing code


using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System;
using System.Linq;
using System.Text.RegularExpressions;

namespace Becnhmarks.ConsoleX
{
    internal class Program
    {
        static void Main(string[] args)
        {
            BenchmarkRunner.Run<MyClass>();
        }
    }

    public class MyClass
    {
        private readonly string data = @"parent1\..parent2\..parent3\.";
        private string[] parent;
        
        private static readonly Regex splitCompiledRegex = new Regex(@"(?<!\\)\.", RegexOptions.Compiled);

        [Benchmark]
        public string OriginalSplit() => GetPropertyNameOriginal(data, out parent);

        [Benchmark]
        public string NewSplit1() => GetPropertyNameNew1(data, out parent);


        public static string GetPropertyNameOriginal(string name, out string[] parentPath)
        {

            string propertyName = name;
            parentPath = new string[0];

            if (!string.IsNullOrEmpty(propertyName))
            {
                string[] hierarchy = Regex.Split(propertyName, @"(?<!\\)\.")
                    .Select(p => p?.Replace("\\.", ".")).ToArray();
                if (hierarchy.Length > 1)
                {
                    propertyName = hierarchy.Last();
                    parentPath = hierarchy.Take(hierarchy.Length - 1).ToArray();
                }
            }

            return propertyName;
        }

        public static string GetPropertyNameNew1(string name, out string[] parentPath)
        {

            string propertyName = name;
            parentPath = new string[0];

            if (!string.IsNullOrEmpty(propertyName))
            {
                string[] hierarchy = splitCompiledRegex.Split(propertyName);
                for (int i = 0; i < hierarchy.Length; i++)
                {
                    hierarchy[i] = hierarchy[i]?.Replace("\\.", ".");
                }

                if (hierarchy.Length > 1)
                {
                    propertyName = hierarchy[hierarchy.Length - 1];
                    Array.Resize(ref hierarchy, hierarchy.Length - 1);
                    parentPath = hierarchy;
                }
            }

            return propertyName;
        }

    }
}

Copy link
Member

@ArthurMa1978 ArthurMa1978 left a comment

Choose a reason for hiding this comment

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

LGTM, and remove the Regex should be better

@AlexGhiondea
Copy link
Contributor

@PiDiBi thanks for your PR!

Do you happen to have the benchmarks for allocations? Did they change?

@PiDiBi
Copy link
Member Author

PiDiBi commented Mar 18, 2022

should I delete this PR? nobody interested in performance improvements?

@PiDiBi
Copy link
Member Author

PiDiBi commented Apr 11, 2022

hallo, anyone here interested in performance improvement?

@PiDiBi PiDiBi closed this Apr 11, 2022
@archerzz archerzz reopened this Apr 12, 2022
@archerzz
Copy link
Member

archerzz commented Apr 12, 2022

@PiDiBi LGTM. Merging. Thanks for your contribution!

@PiDiBi thanks for your PR!

Do you happen to have the benchmarks for allocations? Did they change?

@AlexGhiondea I re-run the benchmark test with [MemoryDiagnoser]. I think the memory footprint is improved as well.

Method Mean Error StdDev Gen 0 Allocated
OriginalSplit 918.6 ns 18.68 ns 53.00 ns 0.1335 584 B
NewSplit1 500.5 ns 10.00 ns 27.21 ns 0.1011 440 B

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.

4 participants