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

Optimize StemmerUtil for ReadOnlySpan<char>/Span<char>, #1140 #1144

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

paulirwin
Copy link
Contributor

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a change, please open an issue to discuss the change or find an existing issue.

Optimize StemmerUtil for ReadOnlySpan<char>/Span<char>

Fixes #1140

Description

This PR optimizes StemmerUtil's EndsWith and StartsWith for ReadOnlySpan<char>, and Delete and DeleteN for Span<char>. Additionally, this type was missing unit tests (including in latest Lucene AFAICT), so this adds some lucenenet-specific unit tests for this type.

Note that the len parameters are not quite what you might expect on a naïve reading of the XML doc comments. It is not always equal to the input buffer length (otherwise we could just use s.Length and drop the parameter). Instead, it's Lucene's equivalent of what we would call a Slice in .NET. Any characters after len chars in the input buffer are treated as if they aren't there. While we could update every callsite of these methods to pass in a slice of the input buffer (via .AsSpan(0, len)) and drop the parameter, there are over 200 uses of these methods that would have to be updated (and kept in sync in future ports), and the additional overhead of creating an extra slice should be negligible as it's a stack-allocated ref struct. So I figured it would be best to keep the method signatures as close to the original as possible, without removing the len parameter.

@paulirwin paulirwin added the notes:performance-improvement Contains a performance improvement label Mar 17, 2025
@paulirwin paulirwin requested a review from NightOwl888 March 17, 2025 03:24
@paulirwin paulirwin marked this pull request as ready for review March 17, 2025 03:53
@paulirwin
Copy link
Contributor Author

paulirwin commented Mar 17, 2025

Ran some benchmarks and this shows a notable improvement for StartsWith and EndsWith (net9.0, macOS, arm64). Delete/DeleteN didn't see a huge change because the Arrays.Copy method it uses is already optimized for span, but at least it's a more consistent API with the other StemmerUtil methods (and there was a small improvement nonetheless).

(Edit: the small improvement in the Delete(N) Span versions might be due to not having to do the Debugging.AssertsEnabled check, which could not be done from my benchmark project as that's an internal type. The benchmark results are thus effectively identical for Delete(N). The CharArray versions were testing the beta 17 NuGet package.)


| Method              | Mean        | Error     | StdDev    | Median      | Gen0   | Allocated |
|-------------------- |------------:|----------:|----------:|------------:|-------:|----------:|
| StartsWithCharArray |  2,504.6 ns |  42.41 ns |  73.16 ns |  2,472.3 ns | 0.6447 |   3.95 KB |
| StartsWithSpan      |    636.0 ns |   3.97 ns |   3.52 ns |    635.8 ns | 0.3223 |   1.98 KB |
| EndsWithCharArray   |  2,533.8 ns |  49.05 ns |  63.79 ns |  2,504.3 ns | 0.6447 |   3.95 KB |
| EndsWithSpan        |    836.1 ns |  12.62 ns |  11.18 ns |    834.6 ns | 0.3223 |   1.98 KB |
| DeleteCharArray     | 21,956.9 ns | 305.08 ns | 456.63 ns | 21,742.9 ns | 0.6409 |   3.95 KB |
| DeleteSpan          | 21,900.1 ns | 157.66 ns | 139.76 ns | 21,840.0 ns | 0.6409 |   3.95 KB |
| DeleteNCharArray    | 22,027.7 ns | 345.74 ns | 288.71 ns | 21,953.6 ns | 0.6409 |   3.95 KB |
| DeleteNSpan         | 21,690.5 ns |  68.90 ns |  57.54 ns | 21,681.4 ns | 0.6409 |   3.95 KB |
using System;
using BenchmarkDotNet.Attributes;
using Lucene.Net.Analysis.Util;

namespace LuceneNetBenchmarkPlayground;

[MemoryDiagnoser]
public class BenchmarkLucene
{
    public static bool StartsWith(ReadOnlySpan<char> s, int len, ReadOnlySpan<char> prefix)
    {
        int prefixLen = prefix.Length;
        if (prefixLen > len)
        {
            return false;
        }
        return s.StartsWith(prefix, StringComparison.Ordinal);
    }

    public static bool EndsWith(ReadOnlySpan<char> s, int len, ReadOnlySpan<char> suffix)
    {
        int suffixLen = suffix.Length;
        if (suffixLen > len)
        {
            return false;
        }

        return s.Slice(0, len).EndsWith(suffix, StringComparison.Ordinal);
    }

    public static int Delete(Span<char> s, int pos, int len)
    {
        //if (Debugging.AssertsEnabled) Debugging.Assert(pos < len);
        if (pos < len - 1) // don't arraycopy if asked to delete last character
        {
            // Arrays.Copy(s, pos + 1, s, pos, len - pos - 1);
            s.Slice(pos + 1, len - pos - 1).CopyTo(s.Slice(pos, len - pos - 1));
        }
        return len - 1;
    }

    public static int DeleteN(Span<char> s, int pos, int len, int nChars)
    {
        //if (Debugging.AssertsEnabled) Debugging.Assert(pos + nChars <= len);
        if (pos + nChars < len) // don't arraycopy if asked to delete the last characters
        {
            // Arrays.Copy(s, pos + nChars, s, pos, len - pos - nChars);
            s.Slice(pos + nChars, len - pos - nChars).CopyTo(s.Slice(pos, len - pos - nChars));
        }
        return len - nChars;
    }

    [Benchmark]
    public void StartsWithCharArray()
    {
        char[] s = new string('f', 1000).ToCharArray();

        for (int i = 0; i < 1000; i++)
        {
            StemmerUtil.StartsWith(s, i, "fff");
        }
    }

    [Benchmark]
    public void StartsWithSpan()
    {
        ReadOnlySpan<char> s = new string('f', 1000);

        for (int i = 0; i < 1000; i++)
        {
            StartsWith(s, i, "fff");
        }
    }

    [Benchmark]
    public void EndsWithCharArray()
    {
        char[] s = new string('f', 1000).ToCharArray();

        for (int i = 0; i < 1000; i++)
        {
            StemmerUtil.EndsWith(s, i, "fff");
        }
    }

    [Benchmark]
    public void EndsWithSpan()
    {
        ReadOnlySpan<char> s = new string('f', 1000);

        for (int i = 0; i < 1000; i++)
        {
            EndsWith(s, i, "fff");
        }
    }

    [Benchmark]
    public void DeleteCharArray()
    {
        char[] s = new string('f', 1000).ToCharArray();

        for (int i = 0; i < 1000; i++)
        {
            StemmerUtil.Delete(s, i, 1000);
        }
    }

    [Benchmark]
    public void DeleteSpan()
    {
        char[] s = new string('f', 1000).ToCharArray();

        for (int i = 0; i < 1000; i++)
        {
            Delete(s, i, 1000);
        }
    }

    [Benchmark]
    public void DeleteNCharArray()
    {
        char[] s = new string('f', 1000).ToCharArray();

        for (int i = 0; i < 1000; i++)
        {
            StemmerUtil.DeleteN(s, i, 1000, 2);
        }
    }

    [Benchmark]
    public void DeleteNSpan()
    {
        char[] s = new string('f', 1000).ToCharArray();

        for (int i = 0; i < 1000; i++)
        {
            DeleteN(s, i, 1000, 2);
        }
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes:performance-improvement Contains a performance improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimize StemmerUtil for ReadOnlySpan<char>
1 participant