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

Improve Regex to Reduce Matching Time #4160

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

thomhurst
Copy link
Contributor

Partially fixes #4159

@nohwnd
Copy link
Member

nohwnd commented Nov 27, 2024

The new regex will not match the first line (or the other lines shown in red)

current:
image

new:
image

@Youssef1313
Copy link
Member

@nohwnd That means we need better test coverage :)

@nohwnd
Copy link
Member

nohwnd commented Nov 27, 2024

ofc

@nohwnd
Copy link
Member

nohwnd commented Nov 27, 2024

I thought it would be caught by OutputFormattingIsCorrect test, but that runs only on code we have symbols for, so the line is there. So probably best to add unit test for this, once we figure out what the problem is, if that is just the system being really slow, or some input throwing the regex into a really bad state.

@thomhurst
Copy link
Contributor Author

Ah good shout! I've just added a test for the regex. What do you think?

@nohwnd
Copy link
Member

nohwnd commented Nov 27, 2024

The test looks good.

@thomhurst
Copy link
Contributor Author

A very marginal improvement on the regex performance, but an improvement none-the-less:

https://github.com/thomhurst/MSTP-Stacktrace-Regex-Benchmark/actions/runs/12053547703/job/33609479485#step:4:660

@thomhurst thomhurst changed the title Simplify Regex to Reduce Matching Time Improve Regex to Reduce Matching Time Nov 27, 2024
@Evangelink
Copy link
Member

@thomhurst I haven't followed in details teh discussion. Is the PR already handling the timeout case that you reported in the issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.Text.RegularExpressions.RegexMatchTimeoutException
5 participants