GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-107369: optimize textwrap.indent() by methane · Pull Request #107374 · python/cpython

Conversation

@methane

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

  • Issue: Optimize textwrap.indent() #107369

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gh640 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
methane added 2 commits July 28, 2023 13:36
@methane
optimize textwrap.indent()
94ab051
@methane
Add NEWS
8c5896c
@bedevere-bot bedevere-bot mentioned this pull request Jul 28, 2023
Optimize textwrap.indent() #107369
Closed
@bedevere-bot bedevere-bot added the awaiting core review label Jul 28, 2023
@methane methane added performance Performance or resource usage stdlib Standard Library Python modules in the Lib/ directory labels Jul 28, 2023
@methane
Add what's new entry
6ee731c
eendebakpt
eendebakpt approved these changes Jul 28, 2023
View reviewed changes

@eendebakpt eendebakpt left a comment

Copy link
Copy Markdown
Contributor

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.

All reactions
serhiy-storchaka
serhiy-storchaka reviewed Jul 28, 2023
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

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.

All reactions
@methane

methane commented Jul 28, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@methane
Use lstrip instead of strip
fad98a2
@serhiy-storchaka

serhiy-storchaka commented Jul 28, 2023

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

eendebakpt
eendebakpt reviewed Jul 28, 2023
View reviewed changes
Comment thread Lib/textwrap.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@methane
avoid temporary tuple.
4c6a46a
@methane

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@methane

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

methane added 2 commits July 29, 2023 12:34
@methane
use str.isspace instead of lstrip
5e60878
@methane
add comment about splitlines(True)
16e3dbd
serhiy-storchaka
serhiy-storchaka approved these changes Jul 29, 2023
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

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.

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Jul 29, 2023
@methane
25% -> 30%
734fd01
@methane
methane enabled auto-merge (squash) July 29, 2023 06:03
@methane
methane merged commit 37551c9 into python:main Jul 29, 2023
@methane
methane deleted the opt-textwrap-indent branch July 29, 2023 06:37
@bedevere-bot bedevere-bot removed the awaiting merge label Jul 29, 2023
@picnixz

picnixz commented Jul 29, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Jul 29, 2023
Optimize textwrap.indent a bit more. #107424
Closed
gh-107424: avoid using lambda functions in textwrap.indent() #107426
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eendebakpt eendebakpt eendebakpt approved these changes
@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

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

Projects

None yet

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.

5 participants

@methane @serhiy-storchaka @picnixz @eendebakpt @bedevere-bot
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.