-
-
Notifications
You must be signed in to change notification settings - Fork 32.7k
gh-130167: Optimise textwrap.dedent()
#131919
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
Merged
Merged
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
be59df8
Add whitespace tests
AA-Turner fec6717
Optimise ``textwrap.dedent()``
AA-Turner 19e76d3
Further micro-optimisation for entirely blank input
AA-Turner 850c07a
whitespace
AA-Turner 796fb3e
Use margin as the loop index variable
AA-Turner ecc9cfc
Add full stops
AA-Turner 311ac87
Further micro-optimisation suggested by Pieter Eendebak
AA-Turner f2050ae
Remove early-exit check for whitespace-only input, as suggested by Pi…
AA-Turner 1f73bcb
More test updates
AA-Turner cc67165
Update NEWS
AA-Turner bf58dfa
Update NEWS
AA-Turner File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 2 additions & 0 deletions
2
Misc/NEWS.d/next/Library/2025-03-30-19-55-10.gh-issue-131792.NNjzFA.rst
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Improved performance of :func:`textwrap.dedent` by ~2.4x. | ||
Patch by Adam Turner and Marius Juston. |
Oops, something went wrong.
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.
The paths are here because otherwise
non_blank_lines
is empty (not for performance I would say, the cases are rare).It is possible though to get rid of these cases by writing
Not sure whether this makes the code nice and small, or part of some obfuscated code contest :-)
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.
Using:
I get the following benchmark results:
Notably, the synthetic benchmarks contain quite a lot of whitespace-only cases:
Only blank lines
,Edge case: Single indented line only
,Edge case: Empty text
, andwhitespace_only
. The text-heavy benchmarks don't seem to change, and some have slight improvements.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 is looking really good! I am having a lot of difficulties improving this further. The only possible other optimizations I can think of are optimizing the
min
/max
computation to maybe perform the computation as it is computing the non_blank_lines and maybe cache the computation ofnot l.isspace()
since that is done twice. I was trying to implement something like that but did not have much success there since the list comprehension is C optimized as well as themin
andmax
operations.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.
Caching would only make sense for lines that are very very long and for which we need to iterate multiple times over the same line. Now, for caching, we could either use a custom str class to cache the call or cache the results themselves in an other list and use it as a mask (as for numpy arrays) but I don't know if we're gaining something (we need to test that).
I'm almost certain we can compute
isspace
andnon_blank_lines
simultaneously using someitertools
recipes but I don't know how much we gain. In the worst case, we need to iterate over all the lines at least 3 + 3 (min/max + compute margin). Currently, we already iterate over the lines at least 1 + 2 + 1 + 1 times (1 fornon_blank_lines
, 2 formin/max
, 1 for margin, and 1 for the final merge) so we're already doing lots of passes.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 benchmarked a version using
itertools.compress
, and found it was slower overall (1.1x):I suspect that the
isspace()
calls are quite cheap, as I assume they exit on the first non-space character. I also prefer the simplicity of the current implementation. What would help is a C-level min-max function that returns both the min and max from one iteration, but that's out of scope for this PR.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.
yes
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.
Agreed! I was just thinking about potentially creating this function. If I were to try this do you happen to know where the
min
andmax
implementation is in this repo? Should be pretty simple to implement the min-max version from that.