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

[API Proposal]: IndexOfAnyValues<T>.Contains(T) #78722

Closed
MihaZupan opened this issue Nov 22, 2022 · 6 comments · Fixed by #78996
Closed

[API Proposal]: IndexOfAnyValues<T>.Contains(T) #78722

MihaZupan opened this issue Nov 22, 2022 · 6 comments · Fixed by #78996
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory tenet-performance Performance related issue
Milestone

Comments

@MihaZupan
Copy link
Member

MihaZupan commented Nov 22, 2022

Background and motivation

Augmenting the recently-added IndexOfAnyValues APIs: #68328 (comment)

When updating existing code to use the new IndexOfAnyValues API, it is very common to replace the existing needle constant with the IndexOfAnyValues instance.

This is fine if the only operations using said needle were {Last}IndexOfAny{Except} calls, but it is not uncommon to also use that needle for single-value checks Needle.Contains(c). In those cases, you are forced into keeping the original needle around.

As IndexOfAnyValues is also already a representation of values optimized for searching, it can be more efficient than a full linear scan of the needle.

API Proposal

namespace System.Buffers;

public class IndexOfAnyValues<T> where T : IEquatable<T>?
{
    public virtual bool Contains(T value);
}

API Usage

Instead of

private const string Needle = ",[]&*+";
private static readonly IndexOfAnyValues<char> s_needle = IndexOfAnyValues.Create(Needle);

if (Needle.Contains(c))
{
    // ...
}

you can do

private static readonly IndexOfAnyValues<char> s_needle = IndexOfAnyValues.Create(",[]&*+");

if (s_needle.Contains(c))
{
    // ...
}

Alternative Designs

No response

Risks

No response

@MihaZupan MihaZupan added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Memory tenet-performance Performance related issue labels Nov 22, 2022
@ghost
Copy link

ghost commented Nov 22, 2022

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

Issue Details

Background and motivation

When updating existing code to use the new IndexOfAnyValues API, it is very common to replace the existing needle constant with the IndexOfAnyValues instance.
This is fine if the only operations using said needle were {Last}IndexOfAny{Except} calls, but it is not uncommon to also use that needle for single-value checks Needle.Contains(c). In those cases, you are forced into keeping the original needle around.

private const string Needle = ",[]&*+";
private static readonly IndexOfAnyValues<char> s_needle = IndexOfAnyValues.Create(Needle);

if (Needle.Contains(c))
{
    // ...
}

As IndexOfAnyValues is also already a representation of values optimized for searching, it can be more efficient than a full linear scan of the needle.

private static readonly IndexOfAnyValues<char> s_needle = IndexOfAnyValues.Create(",[]&*+");

if (s_needle.Contains(c))
{
    // ...
}

API Proposal

namespace System.Buffers;

public class IndexOfAnyValues<T> where T : IEquatable<T>?
{
    public bool Contains(T value);
}

API Usage

Instead of

private const string Needle = ",[]&*+";
private static readonly IndexOfAnyValues<char> s_needle = IndexOfAnyValues.Create(Needle);

if (Needle.Contains(c))
{
    // ...
}

you can do

private static readonly IndexOfAnyValues<char> s_needle = IndexOfAnyValues.Create(",[]&*+");

if (s_needle.Contains(c))
{
    // ...
}

Alternative Designs

No response

Risks

No response

Author: MihaZupan
Assignees: -
Labels:

api-suggestion, area-System.Memory, tenet-performance

Milestone: -

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 22, 2022
@MihaZupan
Copy link
Member Author

cc: @stephentoub

@jkotas
Copy link
Member

jkotas commented Nov 22, 2022

Alternative Designs

Should we have a different type for this? Searching for a single match in a fixed set seems to be very different operation from searching for a fixed set match in a sequence.

@stephentoub
Copy link
Member

We could, though the few examples I've seen so far have been related enough that I think it makes sense to have this on the same type (plus, the instance has also likely already precomputed the state to do the single check quickly due to needing it for scalar IndexOfAny operations). Examples have been of the form:

int pos = data.IndexOfAny(s_values);
if (pos < 0)
    return data;

data = data.Slice(pos);
foreach (char c in data)
{
    if (s_values.Contains(c))
        Process(c);
}

so basically using the values for both a fast path and a slow path.

@MihaZupan
Copy link
Member Author

MihaZupan commented Nov 23, 2022

Another example where this API would be really useful: improving Uri.Escape helpers MihaZupan@a5027e1

A few other cases:

@MihaZupan MihaZupan added this to the 8.0.0 milestone Nov 28, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Nov 28, 2022
@MihaZupan MihaZupan added api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Nov 28, 2022
@terrajobst
Copy link
Member

terrajobst commented Nov 29, 2022

Video

  • Looks good but we'd prefer if the public method weren't virtual, i.e. the virtuality would be hidden.
namespace System.Buffers;

public class IndexOfAnyValues<T> where T : IEquatable<T>?
{
    public bool Contains(T value);
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Nov 29, 2022
@MihaZupan MihaZupan removed the blocking Marks issues that we want to fast track in order to unblock other important work label Nov 29, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 29, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 30, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory tenet-performance Performance related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants