KEMBAR78
Use IndexOfAny{Except}InRange in RegexCompiler / source generator by stephentoub · Pull Request #76859 · dotnet/runtime · GitHub
Skip to content

Use IndexOfAny{Except}InRange in RegexCompiler / source generator #76859

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 27, 2022
Merged

Use IndexOfAny{Except}InRange in RegexCompiler / source generator #76859

merged 2 commits into from
Oct 27, 2022

Conversation

@stephentoub
Copy link
Member

Depends on #76803.

This augments our existing use of IndexOf, IndexOfAny, and IndexOfAnyExcept to also support IndexOfAnyInRange and IndexOfAnyExceptInRange. That means, for example, we can now efficiently find the start of a pattern like [0-9]{5}, via a vectorized search, whereas previously it'd require iterating character by character in a scalar loop.

As part of this, I changed some tuples to instead be named structs. They were becoming unwieldy, and we expect we'll be adding even more here as additional IndexOf variants become available.

@stephentoub stephentoub added this to the 8.0.0 milestone Oct 11, 2022
@stephentoub stephentoub requested a review from joperezr October 11, 2022 04:58
@ghost ghost assigned stephentoub Oct 11, 2022
@ghost
Copy link

ghost commented Oct 11, 2022

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

Issue Details

Depends on #76803.

This augments our existing use of IndexOf, IndexOfAny, and IndexOfAnyExcept to also support IndexOfAnyInRange and IndexOfAnyExceptInRange. That means, for example, we can now efficiently find the start of a pattern like [0-9]{5}, via a vectorized search, whereas previously it'd require iterating character by character in a scalar loop.

As part of this, I changed some tuples to instead be named structs. They were becoming unwieldy, and we expect we'll be adding even more here as additional IndexOf variants become available.

Author: stephentoub
Assignees: -
Labels:

area-System.Text.RegularExpressions, tenet-performance
Milestone: 8.0.0

joperezr
joperezr approved these changes Oct 11, 2022
View reviewed changes
Copy link
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

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

I assume the tests we have currently exercise this code already for both source generator and compiled engine.

Left a few nits, but this otherwise LGTM assuming CI will be green once the change in System.Memory goes in.

@joperezr
Copy link
Member

BTW, just curious, did you happen to run our perf tests to measure any gains that we might have gotten from this?

@stephentoub stephentoub added the blocked Issue/PR is blocked on something - see comments label Oct 12, 2022
@stephentoub
Use IndexOfAny{Except}InRange in RegexCompiler / source generator
b344c01 
This augments our existing use of IndexOf, IndexOfAny, and IndexOfAnyExcept to also support IndexOfAnyInRange and IndexOfAnyExceptInRange.  That means, for example, we can now efficiently find the start of a pattern like `[0-9]{5}`, via a vectorized search, whereas previously it'd require iterating character by character in a scalar loop.

As part of this, I changed some tuples to instead be named structs.  They were becoming unwieldy, and we expect we'll be adding even more here as additional IndexOf variants become available.
@stephentoub
Address PR feedback
2036504 
And add a bit more test coverage
@stephentoub stephentoub removed the blocked Issue/PR is blocked on something - see comments label Oct 26, 2022
This was referenced Oct 27, 2022
Closed
Closed
@stephentoub
Copy link
Member Author

BTW, just curious, did you happen to run our perf tests to measure any gains that we might have gotten from this?

I didn't. I don't think any of our perf tests have patterns impacted by this. I'll pay attention to any improvements/regression reports, though, in case one slips through.

@stephentoub
Copy link
Member Author

I assume the tests we have currently exercise this code already for both source generator and compiled engine.

Yup. Though I added a few more tests to cover a few gaps.

@stephentoub stephentoub merged commit f1a093f into dotnet:main Oct 27, 2022
@stephentoub stephentoub deleted the regexindexofrange branch October 27, 2022 04:11
@AndyAyersMS AndyAyersMS mentioned this pull request Nov 1, 2022
Closed
@EgorBo EgorBo mentioned this pull request Nov 3, 2022
Closed
@AndyAyersMS
Copy link
Member

Improvements:

@kunalspathak kunalspathak mentioned this pull request Nov 4, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@MihaZupan MihaZupan MihaZupan left review comments

@joperezr joperezr joperezr approved these changes

Assignees

@stephentoub stephentoub

Labels

area-System.Text.RegularExpressions tenet-performance Performance related issue

Projects

None yet

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@stephentoub @joperezr @AndyAyersMS @MihaZupan