Skip to content

Let's not create regex for every function call. #1384

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

Closed
wants to merge 2 commits into from

Conversation

Ivanidzo4ka
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka commented Oct 25, 2018

Maybe this would speed up our tests?

#1382
#1348

@@ -504,16 +504,17 @@ protected bool CheckEqualityFromPathsCore(string relPath, string basePath, strin
}
}

private static Regex MatchNumbers = new Regex(@"\b[0-9]+\.?[0-9]*(E-[0-9]*)?\b", RegexOptions.IgnoreCase | RegexOptions.Compiled);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

make it readonly.

Copy link
Contributor

@Anipik Anipik left a comment

Choose a reason for hiding this comment

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

LGTM, you may need to rebase it after #1320

@Ivanidzo4ka
Copy link
Contributor Author

@Anipik let's see whose tests get finished first :D

Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

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

Wow in Windows_Debug Microsoft.ML.Runtime.RunTests.TestPredictors now took 5 mins 👏

@Ivanidzo4ka
Copy link
Contributor Author

Sorry, I've create this PR on top of branch in this repo, so now it's a bit tricky to modify. So I rather create new PR with branch in my fork and close this one.
New one: #1388

@Ivanidzo4ka Ivanidzo4ka deleted the Ivanidze/Small_speed_up_in_tests branch October 26, 2018 00:11
@ghost ghost locked as resolved and limited conversation to collaborators Mar 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants