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

[Analyzer] Use IndexOfAnyValues instead of inlined or cached array #78587

Closed
stephentoub opened this issue Nov 19, 2022 · 5 comments · Fixed by dotnet/roslyn-analyzers#6898
Closed
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer tenet-performance Performance related issue
Milestone

Comments

@stephentoub
Copy link
Member

stephentoub commented Nov 19, 2022

#68328 is adding a new caching mechanism for IndexOfAny, such that instead of:

private static readonly char[] s_values = new[] { ... }
...
int index = span.IndexOfAny(s_values);

you can write:

private static readonly IndexOfAnyValues<char> s_values = IndexOfAnyValues.Create(new[] { ... }); 
...
int index = span.IndexOfAny(s_values);

We should consider an info-level performance analyzer that will flag cases like that above or where IndexOfAny is being called with an inline array (e.g. IndexOfAny(new[] { ... })) (or with a string constant containing the characters), and recommend switching to use IndexOfAnyValues, ideally with a fixer to automatically make the change.

@stephentoub stephentoub added tenet-performance Performance related issue code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer labels Nov 19, 2022
@stephentoub stephentoub added this to the 8.0.0 milestone Nov 19, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@stephentoub
Copy link
Member Author

cc: @MihaZupan, @marklio

@ghost
Copy link

ghost commented Nov 19, 2022

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

Issue Details

#68328 is adding a new caching mechanism for IndexOfAny, such that instead of:

private static readonly char[] s_values = new[] { ... }
...
int index = span.IndexOfAny(s_values);

you can write:

private static readonly IndexOfAnyValues<char> s_values = IndexOfAnyValues.Create(new[] { ... }); 
...
int index = span.IndexOfAny(s_values);

We should consider an info-level performance analyzer that will flag cases like that above or where IndexOfAny is being called with an inline array (e.g. IndexOfAny(new[] { ... })) (or with a string constant containing the characters), and recommend switching to use IndexOfAnyValues, ideally with a fixer to automatically make the change.

Author: stephentoub
Assignees: -
Labels:

area-System.Memory, tenet-performance, code-analyzer, code-fixer

Milestone: 8.0.0

@stephentoub stephentoub added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 21, 2022
@stephentoub stephentoub added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jan 24, 2023
@bartonjs
Copy link
Member

bartonjs commented Mar 7, 2023

Video

The analyzer looks good as proposed.

Category: Performance
Severity: Info (which may get lowered based on the amount of noise in testing/development)

@bartonjs bartonjs 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 Mar 7, 2023
@buyaa-n buyaa-n added the help wanted [up-for-grabs] Good issue for external contributors label May 4, 2023
@tannergooding tannergooding modified the milestones: 8.0.0, Future Jul 24, 2023
@MihaZupan MihaZupan self-assigned this Aug 23, 2023
@MihaZupan MihaZupan removed the help wanted [up-for-grabs] Good issue for external contributors label Aug 23, 2023
@MihaZupan MihaZupan modified the milestones: Future, 8.0.0 Sep 8, 2023
@MihaZupan
Copy link
Member

Marking as 8.0 given we're backporting it in dotnet/roslyn-analyzers#6924

@ghost ghost locked as resolved and limited conversation to collaborators Oct 8, 2023
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 code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer tenet-performance Performance related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants