Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
(Those numbers are very close to what you get by combining the latest
reperf 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
pyperformanceregular expression benchmarks" (or similar) might suffice.Uh oh!
There was an error while loading. Please reload this page.
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.