Optimize recursion of System.Net.Http.HttpRuleParser.GetExpressionLength #35959

MarcoRossignoli · 2019-03-11T18:12:23Z

Fixing past bug I found a unuseful ref param passed to GetQuotedStringLength used by GetCommentLength/GetQuotedStringLength parser.
I tried to remove ref to improve performance thank's to less dereferencing and better usage of registers.
I wrote a test

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Configs;
using System.Collections.Generic;
using System.Net.Http;

namespace HttpRulerParser
{
    [GroupBenchmarksBy(BenchmarkLogicalGroupRule.ByCategory)]
    [CategoriesColumn]
    public class HttpRuleParserComment
    {
        [Params(100)]
        public int Count { get; set; }

        public IEnumerable<string> Comments => new[]
        {
            "Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; Trident/7.0)",
            "((comment)(comment(comment)))"
        };


        public IEnumerable<string> QuotedStrings => new[]
        {
            "token \"quoted string\" token",
            "\"a\\ü\\\"b\\\"c\\\"\\\"d\\\"\""
        };

        [BenchmarkCategory("Comment"), Benchmark(Baseline = true)]
        [ArgumentsSource(nameof(Comments))]
        public void GetCommentLength_Old(string comment)
        {
            for (int i = 0; i < Count; i++)
            {
                HttpRuleParserOld.GetCommentLength(comment, 0, out int len);
            }
        }

        [BenchmarkCategory("Comment"), Benchmark()]
        [ArgumentsSource(nameof(Comments))]
        public void GetCommentLength_New(string comment)
        {
            for (int i = 0; i < Count; i++)
            {
                HttpRuleParserNew.GetCommentLength(comment, 0, out int len);
            }
        }

        [BenchmarkCategory("Quoted"), Benchmark(Baseline = true)]
        [ArgumentsSource(nameof(QuotedStrings))]
        public void GetQuotedStringLength_Old(string quotedString)
        {
            for (int i = 0; i < Count; i++)
            {
                HttpRuleParserOld.GetQuotedStringLength(quotedString, 0, out int len);
            }
        }

        [BenchmarkCategory("Quoted"), Benchmark()]
        [ArgumentsSource(nameof(QuotedStrings))]
        public void GetQuotedStringLength_New(string quotedString)
        {
            for (int i = 0; i < Count; i++)
            {
                HttpRuleParserNew.GetQuotedStringLength(quotedString, 0, out int len);
            }
        }
    }
}

Results

BenchmarkDotNet=v0.11.4, OS=Windows 10.0.17134.590 (1803/April2018Update/Redstone4)
Intel Core i7 CPU 860 2.80GHz (Nehalem), 1 CPU, 8 logical and 4 physical cores
Frequency=2727538 Hz, Resolution=366.6310 ns, Timer=TSC
.NET Core SDK=3.0.100-preview3-010431
  [Host]     : .NET Core 3.0.0-preview3-27503-5 (CoreCLR 4.6.27422.72, CoreFX 4.7.19.12807), 64bit RyuJIT
  DefaultJob : .NET Core 3.0.0-preview3-27503-5 (CoreCLR 4.6.27422.72, CoreFX 4.7.19.12807), 64bit RyuJIT

Method	Categories	Count	quotedString	comment	Mean	Error	StdDev	Ratio	RatioSD
GetQuotedStringLength_Old	Quoted	100	"a\ü"b"c""d""	?	9,284.0 ns	179.270 ns	257.103 ns	1.00	0.00
GetQuotedStringLength_New	Quoted	100	"a\ü"b"c""d""	?	8,387.3 ns	166.098 ns	197.728 ns	0.90	0.03

GetCommentLength_Old	Comment	100	?	((com(...)nt))) [29]	26,042.2 ns	158.638 ns	148.390 ns	1.00	0.00
GetCommentLength_New	Comment	100	?	((com(...)nt))) [29]	22,839.2 ns	84.810 ns	70.820 ns	0.88	0.01

GetCommentLength_Old	Comment	100	?	Mozil(...)/7.0) [63]	1,384.1 ns	24.599 ns	20.541 ns	1.00	0.00
GetCommentLength_New	Comment	100	?	Mozil(...)/7.0) [63]	635.6 ns	9.051 ns	8.467 ns	0.46	0.01

GetQuotedStringLength_Old	Quoted	100	token(...)token [27]	?	1,402.8 ns	19.091 ns	17.858 ns	1.00	0.00
GetQuotedStringLength_New	Quoted	100	token(...)token [27]	?	626.6 ns	3.480 ns	2.906 ns	0.45	0.01

/cc @davidsh @geoffkizer @stephentoub

src/System.Net.Http/src/System/Net/Http/HttpRuleParser.cs

MarcoRossignoli · 2019-03-11T18:53:55Z

/azp run corefx-outerloop-windows

azure-pipelines · 2019-03-11T18:54:14Z

Azure Pipelines successfully started running 1 pipeline(s).

MarcoRossignoli · 2019-03-11T18:54:38Z

/azp run corefx-outerloop-linux

azure-pipelines · 2019-03-11T18:54:49Z

Azure Pipelines successfully started running 1 pipeline(s).

MarcoRossignoli · 2019-03-11T18:54:55Z

/azp run corefx-outerloop-osx

azure-pipelines · 2019-03-11T18:55:11Z

Azure Pipelines successfully started running 1 pipeline(s).

MarcoRossignoli · 2019-03-11T20:56:44Z

Unrelated outerloop fail https://mc.dot.net/#/user/dotnet-bot/pr~2Fdotnet~2Fcorefx~2Frefs~2Fpull~2F35959~2Fmerge/test~2Ffunctional~2Fcli~2F~2Fouterloop~2F/20190311.1/workItem/System.Net.Http.Functional.Tests/analysis/xunit/System.Net.Http.Functional.Tests.SocketsHttpHandler_HttpClientHandler_Proxy_Test~2FAuthProxy__ValidCreds_ProxySendsRequestToServer(proxyAuthScheme:%20Digest,%20secureServer:%20True,%20proxyClosesConnectionAfterFirst407Response:%20True)

MarcoRossignoli · 2019-03-11T20:57:38Z

/azp run corefx-outerloop-windows

azure-pipelines · 2019-03-11T20:57:51Z

Azure Pipelines successfully started running 1 pipeline(s).

MarcoRossignoli · 2019-03-11T22:18:44Z

Unrelated fail again https://mc.dot.net/#/user/dotnet-bot/pr~2Fdotnet~2Fcorefx~2Frefs~2Fpull~2F35959~2Fmerge/test~2Ffunctional~2Fcli~2F~2Fouterloop~2F/20190311.3/workItem/System.Diagnostics.Process.Tests/analysis/xunit/System.Diagnostics.Tests.ProcessTests~2FGetProcessesByName_ProcessName_ReturnsExpected

src/System.Net.Http/src/System/Net/Http/HttpRuleParser.cs

MarcoRossignoli · 2019-03-12T08:58:28Z

/azp run corefx-outerloop-linux

MarcoRossignoli · 2019-03-12T08:58:32Z

/azp run corefx-outerloop-osx

MarcoRossignoli · 2019-03-12T08:58:36Z

/azp run corefx-outerloop-windows

azure-pipelines · 2019-03-12T08:58:41Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2019-03-12T08:58:46Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2019-03-12T08:58:50Z

Azure Pipelines successfully started running 1 pipeline(s).

MarcoRossignoli · 2019-03-12T10:00:08Z

/azp run corefx-ci (macOS x64_Debug)

azure-pipelines · 2019-03-12T10:00:14Z

No pipelines are associated with this pull request.

MarcoRossignoli · 2019-03-12T10:00:49Z

/azp run corefx-ci

azure-pipelines · 2019-03-12T10:01:07Z

Azure Pipelines successfully started running 1 pipeline(s).

MarcoRossignoli · 2019-03-12T10:03:52Z

@safern I tried to re-run CI because one leg fail(corefx-ci (macOS x64_Debug))...but seems that failed leg doesn't run, what I'm missing?

MarcoRossignoli · 2019-03-12T19:23:00Z

@stephentoub PTAL

safern · 2019-03-12T22:57:49Z

@safern I tried to re-run CI because one leg fail(corefx-ci (macOS x64_Debug))...but seems that failed leg doesn't run, what I'm missing?

Running an individual leg through comment is not yet supported. We've requested this feature. I added it under the known issues section in the docs so that I submit a PR to update it whenever it is enabled:

https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/pullrequest-builds.md#known-issues -- number 7

MarcoRossignoli · 2019-03-13T06:40:20Z

@safern you're right I know about doc but I was convinced that I already have, in my dreams maybe 😄 , thank's for the answer.

MarcoRossignoli · 2019-03-14T07:48:11Z

/azp run corefx-outerloop-windows

azure-pipelines · 2019-03-14T07:48:25Z

Azure Pipelines successfully started running 1 pipeline(s).

MarcoRossignoli · 2019-03-14T09:40:40Z

/azp run corefx-outerloop-windows

azure-pipelines · 2019-03-14T09:40:54Z

Azure Pipelines successfully started running 1 pipeline(s).

MarcoRossignoli · 2019-03-14T12:40:26Z

/azp run corefx-outerloop-windows

azure-pipelines · 2019-03-14T12:40:38Z

Azure Pipelines successfully started running 1 pipeline(s).

src/System.Net.Http/src/System/Net/Http/HttpRuleParser.cs

stephentoub

Thanks.

davidsh · 2019-03-14T16:33:57Z

/azp run corefx-outerloop-windows

davidsh · 2019-03-14T16:34:09Z

/azp run corefx-outerloop-linux

azure-pipelines · 2019-03-14T16:34:17Z

Azure Pipelines successfully started running 1 pipeline(s).

davidsh · 2019-03-14T16:34:23Z

/azp run corefx-outerloop-osx

azure-pipelines · 2019-03-14T16:34:26Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2019-03-14T16:34:39Z

Azure Pipelines successfully started running 1 pipeline(s).

…gth (dotnet/corefx#35959) Optimize recursion of System.Net.Http.HttpRuleParser.GetExpressionLength Commit migrated from dotnet/corefx@4636de3

optimize recursion

bf64701

MarcoRossignoli commented Mar 11, 2019

View reviewed changes

src/System.Net.Http/src/System/Net/Http/HttpRuleParser.cs Outdated Show resolved Hide resolved

davidsh added the area-System.Net.Http label Mar 11, 2019

davidsh added this to the 3.0 milestone Mar 11, 2019

stephentoub reviewed Mar 12, 2019

View reviewed changes

src/System.Net.Http/src/System/Net/Http/HttpRuleParser.cs Outdated Show resolved Hide resolved

address PR feedback

ec50287

stephentoub reviewed Mar 14, 2019

View reviewed changes

src/System.Net.Http/src/System/Net/Http/HttpRuleParser.cs Outdated Show resolved Hide resolved

stephentoub approved these changes Mar 14, 2019

View reviewed changes

address PR feedback

5a43d52

davidsh approved these changes Mar 14, 2019

View reviewed changes

davidsh merged commit 4636de3 into dotnet:master Mar 14, 2019

MarcoRossignoli deleted the getexpressionlengthoptimization branch March 14, 2019 18:35

Optimize recursion of System.Net.Http.HttpRuleParser.GetExpressionLength #35959

Optimize recursion of System.Net.Http.HttpRuleParser.GetExpressionLength #35959

Conversation

MarcoRossignoli commented Mar 11, 2019

MarcoRossignoli commented Mar 11, 2019

azure-pipelines bot commented Mar 11, 2019

MarcoRossignoli commented Mar 11, 2019

azure-pipelines bot commented Mar 11, 2019

MarcoRossignoli commented Mar 11, 2019

azure-pipelines bot commented Mar 11, 2019

MarcoRossignoli commented Mar 11, 2019

MarcoRossignoli commented Mar 11, 2019

azure-pipelines bot commented Mar 11, 2019

MarcoRossignoli commented Mar 11, 2019

MarcoRossignoli commented Mar 12, 2019

MarcoRossignoli commented Mar 12, 2019

MarcoRossignoli commented Mar 12, 2019

azure-pipelines bot commented Mar 12, 2019

azure-pipelines bot commented Mar 12, 2019

azure-pipelines bot commented Mar 12, 2019

MarcoRossignoli commented Mar 12, 2019

azure-pipelines bot commented Mar 12, 2019

MarcoRossignoli commented Mar 12, 2019

azure-pipelines bot commented Mar 12, 2019

MarcoRossignoli commented Mar 12, 2019

MarcoRossignoli commented Mar 12, 2019

safern commented Mar 12, 2019

MarcoRossignoli commented Mar 13, 2019 • edited Loading

MarcoRossignoli commented Mar 14, 2019

azure-pipelines bot commented Mar 14, 2019

MarcoRossignoli commented Mar 14, 2019

azure-pipelines bot commented Mar 14, 2019

MarcoRossignoli commented Mar 14, 2019

azure-pipelines bot commented Mar 14, 2019

stephentoub left a comment

Choose a reason for hiding this comment

davidsh commented Mar 14, 2019

davidsh commented Mar 14, 2019

azure-pipelines bot commented Mar 14, 2019

davidsh commented Mar 14, 2019

azure-pipelines bot commented Mar 14, 2019

azure-pipelines bot commented Mar 14, 2019

MarcoRossignoli commented Mar 13, 2019 •

edited

Loading