-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
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
gh-91404: Improve re
performance
#91495
Conversation
Nice speedup. I wonder how much of the speedup would be achieved by just moving
How easy would it be to apply the changes that move |
@brandtbucher, could you show results with disabled computed gotos? Only regex-related benchmarks are interesting. |
With only computed gotos:
With only new locals:
Combined:
For I think we should keep both. |
Excellent! Before merging, could you compare it with 3.10? If there is the same difference, it would be worth to add a note about speed up 10-20% in the NEWS and What's New files. |
@@ -0,0 +1,3 @@ | |||
Improve the performance of :mod:`re` matching by using computed gotos (or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add that it speeds up matching by 10-20%.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't. These claimed speedups are highly misleading.
The release notes for 3.9 and 3.10 have various claimed large speedups, yet 3.9 is no faster than 3.8 and 3.10 only a little bit faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The benchmark results are convincing to me.
Without this it is not clear why bother with making this change at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll only see a 10-20% speedup for these specific benchmarks, which are contrived.
Real programs, even those that spend a lot regexes will spend a much lower proportion of their time in the regex library.
Any number we give will be misleading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This branch vs 3.10:
- regex_v8: 147 ms +- 12 ms -> 132 ms +- 11 ms: 1.11x faster
- regex_effbot: 19.1 ms +- 1.2 ms -> 17.5 ms +- 0.9 ms: 1.10x faster
- regex_dna: 1.30 sec +- 0.01 sec -> 1.24 sec +- 0.01 sec: 1.04x faster
(Those numbers are very close to what you get by combining the latest re
perf regressions with the numbers from this PR.)
I'll just leave the NEWS entry as-is (very few people read it anyways). I don't have a strong opinion on what should go in What's New (but @pablogsal might).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps the wording "up to 10% faster on the pyperformance
regular expression benchmarks" (or similar) might suffice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should indeed go in the What's new. Could you add a small sentence in the Performance section, maybe? Specifying that the improvement is in the re engine and that it only affects re operations? If we need to remove it later is easier than the risk of forgetting to add something.
This is a backport of the upstream 3.11 improvement: python/cpython#91495 I only backported the ctx->pattern -> pattern and ctx->ptr -> ptr part because using computed goto actually decreased perf slightly on the opt build.
This makes a few performance improvements in
sre_lib.h
:ctx
pointer (ctx->pattern
andctx->ptr
) are lifted into local variables.(The diff looks gnarly, but everything inside the main switch is just mechanical replacements to support these two changes. It's not really that bad.)
It yields nice improvements on all of the expected benchmarks, and a 1% improvement overall:
Maybe
re
won't be slower in 3.11 after all! 🙃