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.
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
Miscellaneous render speedups #5225
Miscellaneous render speedups #5225
Changes from 9 commits
7d56907
a7d86c3
0d3a0d4
bd2dc10
0b774d0
31fefdb
0d3a9b8
7355c72
42fc34d
66cd6d8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
These
maxsize=200
are more or less arbitrarily chosen.The default is
128
IIRC, but I just wanted to specify some (reasonable) number such that the temptation to addmaxsize=None
later is lessened.We may not want to use
maxsize=None
deliberately to avoid unbound resource usage downstream, e.,g. in case multiple recipes are rendered in the same process.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.
Memoizing compiled patterns might give a slight advantage here
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.
As mentioned by @dholth below, the
re
module already caches compiled patterns.In contrast to
re
's functions, the wrappers here not only cache for the pattern but also the matched string.This is because we have redundant
re.*(same_pattern, same_string)
calls due to the repeated re-processing mentioned in #5224 (comment) .As such, the memoizations here not try to sidestep the recompilation of patterns but the actual regex search/matches.
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.
Not a fan of this repetition and the pattern of having to cache regex
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.
Personally, I'm also triggered by unnecessary repetitions, but I'm fine with it in this case since it should be a set-and-forget thing.
That said, I would make sense to profile this (for majorly affected cases) again to see which of these memoization wrappers have individually significant impact.
Meaning, we possibly only need a subset of them to get roughly the same results.
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 needs tests to verify that it's doing the right thing
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.
+1 Makes sense to add/extend tests for
select_lines
which ensure repeated calls toselect_lines
and the non-memoized_select_lines
always yield the same results.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.
Enabling CodSpeed benchmarks and adding a new
test_select_lines_battery
test in #5233 so we can better measure improvements