Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

apply StartsWith optimizations to EndsWith#26962

Merged
adamsitnik merged 1 commit intodotnet:masterfrom
adamsitnik:endsWithLinuxPerf
Oct 4, 2019
Merged

apply StartsWith optimizations to EndsWith#26962
adamsitnik merged 1 commit intodotnet:masterfrom
adamsitnik:endsWithLinuxPerf

Conversation

@adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Oct 1, 2019 •
edited
Loading

By reusing the code from #26621 I was able to get similar improvements for EndsWith/IsSuffix.

Using these benchmarks from performance repo we can see that:

  • "hit" - given string ends with given suffix is on average 4 times faster
  • "miss" - last chars are different is now O(1)

BenchmarkDotNet=v0.11.5.1188-nightly, OS=ubuntu 18.04
Intel Xeon CPU E5-2673 v4 2.30GHz, 1 CPU, 4 logical and 2 physical cores
.NET Core SDK=3.1.100-preview1-014415
[Host] : .NET Core 3.1.0 (CoreCLR 4.700.19.50102, CoreFX 4.700.19.50108), X64 RyuJIT
Job-OCNEHW : .NET Core 5.0.0 (CoreCLR 5.0.19.50201, CoreFX 5.0.19.47707), X64 RyuJIT
Job-GGISEQ : .NET Core 5.0.0 (CoreCLR 5.0.19.48001, CoreFX 5.0.19.47707), X64 RyuJIT

PowerPlanMode=00000000-0000-0000-0000-000000000000 IterationTime=250.0000 ms MaxIterationCount=20
MinIterationCount=15 WarmupCount=1

Method Toolchain Options Mean Ratio
IsSuffix_SecondHalf /after/Core_Root/corerun (, IgnoreCase, True) 5,363.90 ns 0.23
IsSuffix_SecondHalf /before/Core_Root/corerun (, IgnoreCase, True) 23,270.77 ns 1.00
IsSuffix_DifferentLastChar /after/Core_Root/corerun (, IgnoreCase, True) 592.83 ns 0.01
IsSuffix_DifferentLastChar /before/Core_Root/corerun (, IgnoreCase, True) 41,711.35 ns 1.00
IsSuffix_SecondHalf /after/Core_Root/corerun (, None, True) 5,372.84 ns 0.23
IsSuffix_SecondHalf /before/Core_Root/corerun (, None, True) 23,449.03 ns 1.00
IsSuffix_DifferentLastChar /after/Core_Root/corerun (, None, True) 498.15 ns 0.01
IsSuffix_DifferentLastChar /before/Core_Root/corerun (, None, True) 42,484.52 ns 1.00
IsSuffix_SecondHalf /after/Core_Root/corerun (en-US, IgnoreCase, True) 5,423.07 ns 0.24
IsSuffix_SecondHalf /before/Core_Root/corerun (en-US, IgnoreCase, True) 22,742.24 ns 1.00
IsSuffix_DifferentLastChar /after/Core_Root/corerun (en-US, IgnoreCase, True) 490.05 ns 0.01
IsSuffix_DifferentLastChar /before/Core_Root/corerun (en-US, IgnoreCase, True) 41,173.58 ns 1.00
IsSuffix_SecondHalf /after/Core_Root/corerun (en-US, None, True) 5,369.36 ns 0.22
IsSuffix_SecondHalf /before/Core_Root/corerun (en-US, None, True) 24,275.39 ns 1.00
IsSuffix_DifferentLastChar /after/Core_Root/corerun (en-US, None, True) 487.19 ns 0.01
IsSuffix_DifferentLastChar /before/Core_Root/corerun (en-US, None, True) 41,989.54 ns 1.00
IsSuffix_SecondHalf /after/Core_Root/corerun (pl-PL, None, False) 6,944.76 ns 0.26
IsSuffix_SecondHalf /before/Core_Root/corerun (pl-PL, None, False) 26,560.62 ns 1.00
IsSuffix_DifferentLastChar /after/Core_Root/corerun (pl-PL, None, False) 436.11 ns 0.009
IsSuffix_DifferentLastChar /before/Core_Root/corerun (pl-PL, None, False) 50,161.53 ns 1.000

@adamsitnik
Copy link
Member Author

adamsitnik commented Oct 1, 2019

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Oct 1, 2019

Azure Pipelines successfully started running 3 pipeline(s).

@adamsitnik
Copy link
Member Author

adamsitnik commented Oct 2, 2019

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Oct 2, 2019

Azure Pipelines successfully started running 3 pipeline(s).

@adamsitnik
Copy link
Member Author

adamsitnik commented Oct 2, 2019

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Oct 2, 2019

Azure Pipelines successfully started running 3 pipeline(s).

@adamsitnik adamsitnik mentioned this pull request Oct 2, 2019
Merged
@adamsitnik
Copy link
Member Author

adamsitnik commented Oct 3, 2019

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Oct 3, 2019

Azure Pipelines successfully started running 3 pipeline(s).

@adamsitnik adamsitnik changed the title [DRAFT] apply StartsWith optimizations to EndsWith apply StartsWith optimizations to EndsWith Oct 3, 2019
@adamsitnik adamsitnik marked this pull request as ready for review October 3, 2019 17:04
@adamsitnik adamsitnik requested a review from tarekgh October 3, 2019 17:04
tarekgh
tarekgh reviewed Oct 3, 2019
View reviewed changes
@tarekgh
Copy link
Member

tarekgh commented Oct 3, 2019

@adamsitnik could you please test the following 2 cases and ensure work as expected with your changes?

            Console.WriteLine($"{CultureInfo.InvariantCulture.CompareInfo.IsSuffix("A\u030A", "Å")}"); // print True
            Console.WriteLine($"{CultureInfo.GetCultureInfo("sk-SK").CompareInfo.IsSuffix("ch", "h")}"); // print False

@adamsitnik
Copy link
Member Author

adamsitnik commented Oct 4, 2019 •
edited
Loading

could you please test the following 2 cases and ensure work as expected with your changes?

Sure! The output:

obraz

EDIT: Btw we have similar test cases in CoreFX test suite so this time I am not sending a PR that adds more test cases:

https://github.com/dotnet/corefx/blob/eff9fbf446468ca411bf5e0475916f316eea7223/src/System.Globalization/tests/CompareInfo/CompareInfoTests.IsSuffix.cs#L33

https://github.com/dotnet/corefx/blob/eff9fbf446468ca411bf5e0475916f316eea7223/src/System.Globalization/tests/CompareInfo/CompareInfoTests.IsSuffix.cs#L47

@adamsitnik adamsitnik merged commit 7d2554f into dotnet:master Oct 4, 2019
@adamsitnik adamsitnik deleted the endsWithLinuxPerf branch October 4, 2019 16:26
SrivastavaAnubhav pushed a commit to SrivastavaAnubhav/coreclr that referenced this pull request Oct 7, 2019
@adamsitnik
apply StartsWith optimizations to EndsWith (dotnet#26962)
7643eff
@lukaaash
Copy link

lukaaash commented Jul 16, 2020

Apparently, in .NET 5.0 Preview 6, value.EndsWith("\0") returns true for all strings (unlike all other .NET platforms). Looks like an unintended side effect or a bug.

@tarekgh
Copy link
Member

tarekgh commented Jul 16, 2020 •
edited
Loading

@lukaaash This is not a bug. in .NET 5.0 we have started to use ICU which has slight collation behavior than Windows NLS. '\0' doesn't have any sort weight. If you don't like this behavior, you may consider using Ordinal comparison instead.

@tarekgh
Copy link
Member

tarekgh commented Jul 16, 2020

CC @safern

@safern
Copy link
Member

safern commented Jul 16, 2020

@lukaaash please refer to: https://docs.microsoft.com/en-us/dotnet/standard/globalization-localization/globalization-icu to get more details about the motivation of the change.

@lukaaash
Copy link

lukaaash commented Jul 21, 2020

@tarekgh @safern Thanks for the explanation and for the link! (In our code where this did cause a problem, we should actually have been using Ordinal comparison anyway.)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@tarekgh tarekgh tarekgh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Globalization tenet-performance Performance related issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@adamsitnik @tarekgh @lukaaash @safern