Call String.Contains(char) from String.Contains(string) for single-char literals

[EgorBo]

Addresses String.Contains performance concerns in this tweet: https://twitter.com/STeplyakov/status/1751858621212991573

Benchmark:

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);

public class MyBenchmarks
{
    readonly string _inputString = 
        "Permission is hereby granted, free of charge, to any person obtaining " +
        "a copy of the Unicode data files and any associated documentation";

    [Benchmark]
    public bool Contains_string() => _inputString.Contains("X");
}

Method	Toolchain	Mean	Ratio
Contains_string	\Core_Root_base\corerun.exe	4.050 ns	1.00
Contains_string	\Core_Root_PR\corerun.exe	2.361 ns	0.58

It uses IsKnownConstant to avoid perf regressions for non-constant and non-single-char input (although, it's probably also beneficial for non-constant and single-char)

[ghost]

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Addresses String.Contains performance concerns in this tweet: https://twitter.com/STeplyakov/status/1751858621212991573

Benchmark:

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);

public class MyBenchmarks
{
    readonly string _inputString = 
        "Permission is hereby granted, free of charge, to any person obtaining " +
        "a copy of the Unicode data files and any associated documentation";

    [Benchmark]
    public bool Contains_string() => _inputString.Contains("X");
}

Method	Toolchain	Mean	Ratio
Contains_string	\Core_Root_base\corerun.exe	4.050 ns	1.00
Contains_string	\Core_Root_PR\corerun.exe	2.361 ns	0.58

It uses IsKnownConstant to avoid perf regressions for non-constant and non-single-char input (although, it's probably also beneficial for non-constant and single-char)

Author:	EgorBo
Assignees:	EgorBo
Labels:	area-System.Runtime
Milestone:	-

[stephentoub]

although, it's probably also beneficial for non-constant and single-char

We already special-case length 1 inside of SpanHelpers.IndexOf. Presumably then the 1.7ns difference here is a fixed overhead involved in the non-inlined IndexOf call and the few extra instructions there, rather than something that grows with the length of the haystack?

[EgorBo]

although, it's probably also beneficial for non-constant and single-char

We already special-case length 1 inside of SpanHelpers.IndexOf. Presumably then the 1.7ns difference here is a fixed overhead involved in the non-inlined IndexOf call and the few extra instructions there, rather than something that grows with the length of the haystack?

That is my understanding as well, I just though that a simple harmless change to eliminate that difference wouldn't hurt

[tannergooding]

We already special-case length 1 inside of SpanHelpers.IndexOf. Presumably then the 1.7ns difference here is a fixed overhead involved in the non-inlined IndexOf call and the few extra instructions there, rather than something that grows with the length of the haystack?

I wonder if there's something we can with regards to PGO or general inlining to help the JIT better understand cases like this.

That is, we have "many" (I believe thousands) of places where we have PublicApi(...) which differs to CoreImpl(...), often there are many overloads of PublicApi(...) which take a fewer number of parameters and do less up front validation where its unnecessary. CoreImpl(...) will then typically special case the short paths itself and we end up with this minor overhead since CoreImpl(...) is typically too large to inline.

In a more ideal world, the JIT would be able to do partial inlining or we'd have CoreImpl(...) set up in a way such that these main dispatching checks can be inlined but the underlying work isn't, to avoid the repeated minor overhead.

[EgorBo]

In a more ideal world, the JIT would be able to do partial inlining

There is also a different/better/more generic way to solve this - inline large methods as is and don't compile cold parts (partial compilation) if their hot part is small according to context-sensitive PGO data. But both options will likely require a lot of efforts 🤷‍♂️ Andy already implemented some basic support for this, only for tier0 for now.

[EgorBo]

WASM failures are known (#95365 and #88148)

[Youssef1313]

@EgorBo Would that mean https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1847 should be removed?

[tannergooding]

https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1847 should be removed?

IMO, no. It's still better, simpler, and overall cheaper to use the explicit overload where it exists. It also doesn't rely on runtime specific optimizations (which may not light up for AOT, Mono, Burst, etc). Where-as if you know you have a single character, using the dedicated API explicitly does the right thing always.

[davepcallan]

Could we do the same with EndsWith / StartsWith?