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

#291 causes drastic performance drop in DetectIdentifiers #331

Closed
cpiber opened this issue Nov 15, 2024 · 4 comments
Closed

#291 causes drastic performance drop in DetectIdentifiers #331

cpiber opened this issue Nov 15, 2024 · 4 comments

Comments

@cpiber
Copy link
Contributor

cpiber commented Nov 15, 2024

We have around 115 equations on which we call DetectIdentifiers. Before (v2.16.1), this took around 50ms. After the change, it takes 17.1 seconds, all of which is spent in evaluating the regex (of which 50% is spent in Match.AddMatch).

Interestingly, #323 improves this somewhat to 16.7 seconds. In any case, a 330 times slowdown seems a bit excessive.

@cpiber
Copy link
Contributor Author

cpiber commented Nov 15, 2024

Turns out the \b is responsible, just adding that in the front drops us back down to 50ms

		private static readonly Regex RootIdentifierDetectionRegex =
-			new Regex(@"(?<id>@?[\p{L}\p{Nl}_][\p{L}\p{Nl}\p{Nd}\p{Mn}\p{Mc}\p{Pc}\p{Cf}_]*)", RegexOptions.Compiled);
+			new Regex(@"\b(?<id>@?[\p{L}\p{Nl}_][\p{L}\p{Nl}\p{Nd}\p{Mn}\p{Mc}\p{Pc}\p{Cf}_]*)", RegexOptions.Compiled);

		private static readonly Regex ChildIdentifierDetectionRegex = new Regex(
-			@"(?<id>@?[\p{L}\p{Nl}_][\p{L}\p{Nl}\p{Nd}\p{Mn}\p{Mc}\p{Pc}\p{Cf}_]*(\.[\p{L}\p{Nl}_][\p{L}\p{Nl}\p{Nd}\p{Mn}\p{Mc}\p{Pc}\p{Cf}_]*)*)",
+			@"\b(?<id>@?[\p{L}\p{Nl}_][\p{L}\p{Nl}\p{Nd}\p{Mn}\p{Mc}\p{Pc}\p{Cf}_]*(\.[\p{L}\p{Nl}_][\p{L}\p{Nl}\p{Nd}\p{Mn}\p{Mc}\p{Pc}\p{Cf}_]*)*)",
			RegexOptions.Compiled);

@metoule can you please comment in more detail why you removed it?

cpiber added a commit to cpiber/DynamicExpresso that referenced this issue Nov 15, 2024
This improves the regex evaluation back to when we still had `\b` in
front, while (hopefully) doing as it should
cpiber added a commit to cpiber/DynamicExpresso that referenced this issue Nov 15, 2024
This improves the regex evaluation back to when we still had `\b` in
front, while (hopefully) doing as it should
@metoule
Copy link
Contributor

metoule commented Nov 15, 2024

@cpiber based on my comment on #291, it was to support identifiers starting with a @ (e.g. @this). I'll check if it can be reintroduced

@cpiber
Copy link
Contributor Author

cpiber commented Nov 15, 2024

@metoule I have something added in #332, please check it, seems I understood your comment right

davideicardi pushed a commit to cpiber/DynamicExpresso that referenced this issue Nov 18, 2024
This improves the regex evaluation back to when we still had `\b` in
front, while (hopefully) doing as it should
davideicardi pushed a commit to cpiber/DynamicExpresso that referenced this issue Nov 18, 2024
This improves the regex evaluation back to when we still had `\b` in
front, while (hopefully) doing as it should
@davideicardi
Copy link
Member

Closed by #332

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

No branches or pull requests

3 participants