FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-107369: optimize textwrap.indent() by methane · Pull Request #107374 · python/cpython · GitHub

/ cpython Public

gh-107369: optimize textwrap.indent() - #107374

Merged
methane merged 8 commits into
python:mainfrom
methane:opt-textwrap-indent
Jul 29, 2023
Merged

gh-107369: optimize textwrap.indent()#107374
methane merged 8 commits into
python:mainfrom
methane:opt-textwrap-indent

Conversation

methane commented Jul 28, 2023
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

indent()-ing Object/unicodeobject.c (15332 lines) about 25% faster.

methane added performance Performance or resource usage stdlib Standard Library Python modules in the Lib/ directory labels Jul 28, 2023

eendebakpt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

methane commented Jul 28, 2023
edited
Loading

Copy link
Copy Markdown
Member Author
  • _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.

Copy link
Copy Markdown
Member

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.

Comment thread Lib/textwrap.py Outdated

methane commented Jul 29, 2023

Copy link
Copy Markdown
Member Author

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

methane commented Jul 29, 2023

Copy link
Copy Markdown
Member Author

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

serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

methane enabled auto-merge (squash) July 29, 2023 06:03
methane merged commit 37551c9 into python:main Jul 29, 2023
methane deleted the opt-textwrap-indent branch July 29, 2023 06:37

picnixz commented Jul 29, 2023
edited
Loading

Copy link
Copy Markdown
Member

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.

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance Performance or resource usage stdlib Standard Library Python modules in the Lib/ directory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL