GeezerVison is showing github.com — gh-107369: optimize textwrap.indent() by methane · Pull Request #107374 · python/cpython
Conversation
methane
commented
Jul 28, 2023
•
edited by bedevere-bot
Loading
Uh oh!
There was an error while loading. Please reload this page.
indent()-ing Object/unicodeobject.c (15332 lines) about 25% faster.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
gh640 reacted with thumbs up emoji
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.
Looks good! Using str.split for the predicate instead of line.strip might change something for input that is not str, but I think this is ok.
Sorry, something went wrong.
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.
lstrip is faster for non-indented lines.
I wonder whether the following variants can be faster for some input and for how wide category of input.
def predicate(line):
return line and (not line[0].isspace() or line.lstrip())
or
predicate = re.compile(r'\S').search
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
methane
commented
Jul 28, 2023
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
_has_nonspace = re.compile(r'\S').search in global and predicate = _has_nonspace -- 3.5ms
- str.rstrip = 1.95ms
- str.lstrip = 2.03ms
lambda x: not x.isspace() = 2.07ms
Since we use splitlines(keepends=True), we can use just not x.isspace(). (no empty line is guaranteed. "".splitlines(keepends=True) == [] and "foo\n".splitlines(True) == ['foo\n']).
But it is a bit tricky and has relatively high cognitive load.
In case of unicodeobject.c, rstrip is bit faster. But it may be because most lines are indented already.
So I chose str.lstrip here, as Serhiy suggested.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Now that you mention it, I can see that using isspace() is the most obvious way to do this. Why I did not see it earlier?
We want to test whether the line has any non-space character. bool(line.strip()) is actually a tricky way -- we strips the line from spaces and if the rest is not empty string, then the original line has non-space characters too. not line.isspace() is a straightforward way -- it asks the opposite question (is the line only contains space characters?) and negates the result.
Algorithmically, isspace() looks more preferable, because it does not create a string. But on practice it may not matter in common cases. Did you compare variants with different inputs? For example Misc/NEWS.d/3.8.0a1.rst may show a very different result.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Now that you mention it, I can see that using isspace() is the most obvious way to do this. Why I did not see it earlier?
Because "".isspace() is False. We need to guarantee that "" is not used here.
x and not x.isspace() would be bit obvious, but little slower.
Algorithmically, isspace() looks more preferable, because it does not create a string. But on practice it may not matter in common cases. Did you compare variants with different inputs? For example Misc/NEWS.d/3.8.0a1.rst may show a very different result.
lstrip() is slow when every line has long indent. But Misc/NEWS.d/3.8.0a1.rst has almost no indents.
With 4c6a46a and https://gist.github.com/methane/5c6153c564d9508199a81c48d33161eb
> ./python.exe bench_indent.py Misc/NEWS.d/3.8.0a1.rst
filename='Misc/NEWS.d/3.8.0a1.rst' 8978 lines.
lstrip: 0.736msec
not x.isspace(): 0.877msec
x and not x.isspace(): 0.929msec
> ./python.exe bench_indent.py Objects/unicodeobject.c
filename='Objects/unicodeobject.c' 15332 lines.
lstrip: 1.812msec
not x.isspace(): 1.877msec
x and not x.isspace(): 1.970msec
If I add text = textwrap.indent(text, " "*32) before bench:
> ./python.exe bench_indent.py Objects/unicodeobject.c
filename='Objects/unicodeobject.c' 15332 lines.
lstrip: 2.259msec
not x.isspace(): 2.356msec
x and not x.isspace(): 2.437msec
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
To maximize performance, we can stop using lambda by...:
if predicate is None:
for line in text.splitlines(True):
if not line.isspace():
prefixed_lines.append(prefix)
prefixed_lines.append(line)
else:
for line in text.splitlines(True):
if predicate(line):
prefixed_lines.append(prefix)
prefixed_lines.append(line)
filename='Objects/unicodeobject.c' 15332 lines.
None: 1.604msec
lstrip: 1.826msec
not x.isspace(): 1.883msec
|
Sorry, something went wrong.
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.
Thank you for your research Inada-san. Which to use here, lstrip or isspace, I leave up to you. It does not really matter in most cases.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
picnixz
commented
Jul 29, 2023
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
For very long texts, I think changing
prefixed_lines = []
for line in text.splitlines(True):
if not line.isspace():
prefixed_lines.append(prefix)
prefixed_lines.append(line)
into the following may improve the overall performances
prefixed_lines = []
append_line = prefixed_lines.append
for line in text.splitlines(True):
if not line.isspace():
append_line(prefix)
append_line(line)
EDIT: After a more careful benchmarking, this does not seem to bring more improvements. However, not using a lambda function seems to be better.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Jul 29, 2023
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Labels
performance
Performance or resource usage
stdlib
Standard Library Python modules in the Lib/ directory
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
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.
|