Skip to content

Use span.Min/Max helpers to speed up SearchValues.Create#129809

Merged
MihaZupan merged 3 commits into
mainfrom
copilot/replace-min-max-loops
Jun 25, 2026
Merged

Use span.Min/Max helpers to speed up SearchValues.Create#129809
MihaZupan merged 3 commits into
mainfrom
copilot/replace-min-max-loops

Conversation

@MihaZupan

Copy link
Copy Markdown
Member

No description provided.

Copilot AI and others added 3 commits June 24, 2026 15:09
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
…y to out params

Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
@MihaZupan MihaZupan added this to the 11.0.0 milestone Jun 24, 2026
@MihaZupan MihaZupan self-assigned this Jun 24, 2026
Copilot AI review requested due to automatic review settings June 24, 2026 17:00
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@MihaZupan

Copy link
Copy Markdown
Member Author

@EgorBot

using System.Buffers;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

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

public class Bench
{
    // Standard Base64 alphabet (64 chars)
    private const string Base64Chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";

    [Benchmark]
    public SearchValues<char> Create_Base64Alphabet() => SearchValues.Create(Base64Chars);
}

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR replaces manual min/max loops with ReadOnlySpan<T>.Min() / .Max() calls (from MemoryExtensions.MinMax) in two places, aiming to streamline and potentially speed up min/max computation used during SearchValues.Create range detection.

Changes:

  • Update SearchValues.TryGetSingleRange<T> to compute minInclusive/maxInclusive via span Min()/Max() and drop the IMinMaxValue<T> constraint.
  • Update regex BDD.SerializeToBytes (debug-only serializer) to validate positivity via Min() and compute the maximum via Max().

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
src/libraries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/Symbolic/BDD.cs Switches debug-only serialization min/max computation to span Min()/Max() helpers.
src/libraries/System.Private.CoreLib/src/System/SearchValues/SearchValues.cs Uses span Min()/Max() in TryGetSingleRange and removes IMinMaxValue<T> constraint.

@MihaZupan MihaZupan enabled auto-merge (squash) June 24, 2026 17:08
@github-actions

Copy link
Copy Markdown
Contributor

Copilot Code Review

Holistic Assessment

Motivation: Justified. The PR replaces manual scalar min/max loops with the built-in vectorized Span.Min()/Span.Max() helpers, which use SIMD-accelerated implementations for primitive types like byte, char, and long. This directly speeds up SearchValues.Create.

Approach: Correct and minimal. The refactoring uses existing, well-tested APIs (MemoryExtensions.Min/Max) that take the vectorized MinMaxInteger path for all involved types. The private method's IMinMaxValue<T> constraint is correctly relaxed since T.MaxValue/T.MinValue are no longer needed.

Summary: ✅ LGTM. Both changes are semantically equivalent to the original code, callers guarantee non-empty spans (so no InvalidOperationException risk), and the constraint relaxation is safe since TryGetSingleRange is private.


Detailed Findings

Detailed Findings

✅ Correctness — Empty span safety verified

Both call sites of TryGetSingleRange guard against empty/single-element spans before the call:

  • Create(ReadOnlySpan<byte>): checks values.IsEmpty and values.Length == 1 first
  • Create(ReadOnlySpan<char>): same guards

So values.Min() / values.Max() will never throw InvalidOperationException due to an empty span.

✅ Correctness — BDD.cs behavioral equivalence

The old code initialized m = 0 and used Math.Max(serialized[i], m), meaning m was implicitly ≥ 0. The new code calls serialized.AsSpan().Max() which returns the actual maximum. Since the Debug.Assert(serialized.AsSpan().Min() > 0) guarantees all elements are strictly positive, the two are equivalent. The two-pass approach (Min for assertion, then Max) is fine here since this method is [ExcludeFromCodeCoverage] and "used only to generate src data files."

✅ Constraint relaxation — Safe for private method

Removing IMinMaxValue<T> from the private TryGetSingleRange method is safe. The constraint was only needed for T.MaxValue/T.MinValue initialization of the loop variables, which are no longer present. No external callers are affected.

✅ Performance path — Vectorized implementations confirmed

For the types used at call sites (byte, char, long), the Min()/Max() methods dispatch to MinMaxInteger<T, ...> which uses SIMD-accelerated implementations, confirming the expected performance improvement over the previous scalar loops.

Note

This review was created by GitHub Copilot.

Generated by Code Review for issue #129809 · ● 19.2M ·

@MihaZupan

Copy link
Copy Markdown
Member Author
Method Toolchain Mean Error Ratio
Create_Base64Bytes PR #129809 109.5 ns 0.13 ns 1.00
Create_Base64Bytes main 150.5 ns 0.11 ns 1.37
Create_Base64Chars PR #129809 111.7 ns 0.18 ns 1.00
Create_Base64Chars main 158.0 ns 0.20 ns 1.42

Not quite as good as #124667, but that makes sense since we're doing two passes for Min/Max instead

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants