Skip to content

Conversation

@AA-Turner
Copy link
Member

@AA-TurnerAA-Turner commented Mar 30, 2025

Alternative to #131792, created as a new PR as I don't want to push commits to @Marius-Juston's main branch, which that PR is created from.

A

@AA-Turner
Copy link
MemberAuthor

AA-Turner commented Mar 30, 2025

Open question: by switching to str.isspace() over the regular expression approach, we now strip '\v'. '\f', and '\r'. The documentation notes that the function Remove[s] any common leading whitespace from every line in text, so this could be seen as a bugfix, but is an observable change in behaviour.

Do we want to restrict the removed whitespace to just tabs and spaces, and update the documentation as appropriate, or is this change acceptable? Previously, there was inconsistent behaviour around Windows-style newlines, which this PR does resolve.

Benchmarkdedent_textwrapdedent_newer
raw_text: no prefix12.1 ms4.81 ms: 2.52x faster
raw_text: "abc \t"12.7 ms4.81 ms: 2.63x faster
raw_text: " "19.4 ms5.77 ms: 3.37x faster
raw_text: "\t"18.4 ms5.66 ms: 3.25x faster
raw_text: "\t abc"21.0 ms5.75 ms: 3.65x faster
raw_text: "\t abc \t "24.1 ms6.12 ms: 3.94x faster
raw_text: 1000 spaces338 ms43.5 ms: 7.76x faster
Basic indented text with empty lines4.66 us2.92 us: 1.60x faster
Text with mixed indentation and blank lines4.28 us2.99 us: 1.43x faster
No indentation (edge case)2.66 us1.95 us: 1.37x faster
Only blank lines1.09 us299 ns: 3.64x faster
Edge case: No common prefix to remove1.79 us1.71 us: 1.05x faster
Edge case: Single indented line2.90 us1.67 us: 1.74x faster
Edge case: Single indented line only704 ns260 ns: 2.71x faster
Edge case: Empty text478 ns130 ns: 3.67x faster
no_indent10.4 us3.68 us: 2.81x faster
spaces18.2 us4.76 us: 3.81x faster
mixed27.5 us18.0 us: 1.53x faster
large_text379 us109 us: 3.47x faster
whitespace_only1.23 us299 ns: 4.12x faster
Geometric mean(ref)2.69x faster

A

@picnixz
Copy link
Member

Can you use python -m pyperf compare_to dedent_textwrap.json dedent_new.json --table so that we have a markdown table (I think that's the option)

@AA-Turner
Copy link
MemberAuthor

Can you use python -m pyperf compare_to dedent_textwrap.json dedent_new.json --table so that we have a markdown table (I think that's the option)

Done, and I reran the benchmarks, now showing a 2.7x improvement.

@eendebakpt
Copy link
Contributor

Open question: by switching to str.isspace() over the regular expression approach, we now strip '\v'. '\f', and '\r'. The documentation notes that the function Remove[s] any common leading whitespace from every line in text, so this could be seen as a bugfix, but is an observable change in behaviour.

This behaviour change is also in the PR by @Marius-Juston. I have no strong opinion on whether the behaviour change is acceptable or not, but it should be in the news entry (and perhaps even in the whats new?).

Lib/textwrap.py Outdated
Comment on lines 429 to 434
if not text:
return text

# If the input is entirely whitespace, return normalized lines.
if text.isspace():
return '\n' * text.count('\n')
Copy link
Contributor

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

def dedent(text): lines = text.split('\n') # Get length of leading whitespace, inspired by ``os.path.commonprefix()``. non_blank_lines = [l for l in lines if l and not l.isspace()] l1 = min(non_blank_lines, default='hello') l2 = max(non_blank_lines, default='world') for margin, c in enumerate(l1): if c != l2[margin] or c not in ' \t': break return '\n'.join([l[margin:] if not l.isspace() else '' for l in lines]) 

Not sure whether this makes the code nice and small, or part of some obfuscated code contest :-)

Copy link
MemberAuthor

@AA-TurnerAA-TurnerMar 30, 2025

Choose a reason for hiding this comment

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

Using:

defdedent(text): lines=text.split('\n') non_blank_lines= [lforlinlinesiflandnotl.isspace()] l1=min(non_blank_lines, default='') l2=max(non_blank_lines, default='') margin=0formargin, cinenumerate(l1): ifc!=l2[margin] orcnotin' \t': breakreturn'\n'.join([l[margin:] ifnotl.isspace() else''forlinlines])

I get the following benchmark results:

Benchmarktextwrapwith whitespace checksproposed (no branches)
raw_text: no prefix12.1 ms4.81 ms: 2.52x faster4.71 ms: 2.58x faster
raw_text: "abc \t"12.7 ms4.81 ms: 2.63x faster4.50 ms: 2.81x faster
raw_text: " "19.4 ms5.77 ms: 3.37x faster5.67 ms: 3.43x faster
raw_text: "\t"18.4 ms5.66 ms: 3.25x faster5.54 ms: 3.32x faster
raw_text: " \t abc"21.0 ms5.75 ms: 3.65x faster5.42 ms: 3.88x faster
raw_text: " \t abc \t "24.1 ms6.12 ms: 3.94x faster5.76 ms: 4.18x faster
raw_text: 1000 spaces338 ms43.5 ms: 7.76x faster42.7 ms: 7.91x faster
Basic indented text with empty lines4.66 us2.92 us: 1.60x faster3.08 us: 1.52x faster
Text with mixed indentation and blank lines4.28 us2.99 us: 1.43x faster3.07 us: 1.39x faster
No indentation (edge case)2.66 us1.95 us: 1.37x faster2.05 us: 1.30x faster
Only blank lines1.09 us299 ns: 3.64x faster1.56 us: 1.43x slower
Edge case: No common prefix to remove1.79 us1.71 us: 1.05x fasternot significant
Edge case: Single indented line2.90 us1.67 us: 1.74x faster1.68 us: 1.73x faster
Edge case: Single indented line only704 ns260 ns: 2.71x faster1.00 us: 1.42x slower
Edge case: Empty text478 ns130 ns: 3.67x faster1.01 us: 2.11x slower
no_indent10.4 us3.68 us: 2.81x faster3.97 us: 2.61x faster
spaces18.2 us4.76 us: 3.81x faster4.57 us: 3.98x faster
mixed27.5 us18.0 us: 1.53x faster16.9 us: 1.63x faster
large_text379 us109 us: 3.47x faster117 us: 3.23x faster
whitespace_only1.23 us299 ns: 4.12x faster1.40 us: 1.14x slower
Geometric mean(ref)2.69x faster1.94x faster

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, and whitespace_only. The text-heavy benchmarks don't seem to change, and some have slight improvements.

Copy link
Contributor

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 of not 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 the min and max operations.

Copy link
Member

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).

isspace= [l.isspace() forlinlines] non_blank_lines= [lines[i] fori, vinenumerate(isspace) ifv] ... return'\n'.join([l[margin:] ifnotisspace[i] else''fori, linenumerate(lines)])

I'm almost certain we can compute isspace and non_blank_lines simultaneously using some itertools 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 for non_blank_lines, 2 for min/max, 1 for margin, and 1 for the final merge) so we're already doing lots of passes.

Copy link
MemberAuthor

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):

# Get length of leading whitespace, inspired by ``os.path.commonprefix()``.non_blank_idx= [landnotl.isspace() forlinlines] non_blank_lines=list(compress(lines, non_blank_idx)) l1=min(non_blank_lines, default='') l2=max(non_blank_lines, default='') margin=0formargin, cinenumerate(l1): ifc!=l2[margin] orcnotin' \t': breakreturn'\n'.join([l[margin:] ifnon_blank_idx[i] else''fori, linenumerate(lines)])

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.

Copy link
Member

Choose a reason for hiding this comment

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

they exit on the first non-space character

yes

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Agreed! I was just thinking about potentially creating this function. If I were to try this do you happen to know where the min and max implementation is in this repo? Should be pretty simple to implement the min-max version from that.

@Marius-Juston
Copy link
Contributor

@AA-Turner, would it be possible to get your benchmarking script, please, so that I can use that as a comparison?

@AA-Turner
Copy link
MemberAuthor

AA-Turner commented Mar 30, 2025

$ python bm_dedent.py --fast --quiet --warmups 3 --output <filename>.json --quiet $ python -m pyperf compare_to <old>.json <new>.json --table --table-format md
bm_dedent.py
fromtextwrapimportindent, dedentastextwrap_dedentimportpyperfdefsetup_cases(): withopen("Objects/unicodeobject.c") asf: raw_text=f.read() return [ ('raw_text: no prefix', raw_text), (r'raw_text: "abc \t"', indent(raw_text, 'abc \t')), ('raw_text: " "', indent(raw_text, ' ')), (r'raw_text: "\t"', indent(raw_text, '\t')), (r'raw_text: " \t abc"', indent(raw_text, ' \t abc')), (r'raw_text: " \t abc \t "', indent(raw_text, ' \t abc \t ')), ("raw_text: 1000 spaces", indent(raw_text, ' '*1000)), ( 'Basic indented text with empty lines', """ def hello(): print("Hello, world!") """, ), ( 'Text with mixed indentation and blank lines', """ This is a test. Another line. """, ), ( 'No indentation (edge case)', """No indents here. Just normal text. With empty lines.""", ), ( 'Only blank lines', """ """, ), ( 'Edge case: No common prefix to remove', """hello world """, ), ( 'Edge case: Single indented line', """ Only one indented line""", ), ( 'Edge case: Single indented line only', """ """, ), ( 'Edge case: Empty text', """""", ), ('no_indent',textwrap_dedent(textwrap_dedent.__doc__)), ('spaces','\n'.join([' '+lforlintextwrap_dedent(textwrap_dedent.__doc__).split('\n')])), ('mixed','\n'.join([' Hello space', '\tHello tab', 'Hello'] *20)), ('large_text','\n'.join([textwrap_dedent.__doc__] *40)), ('whitespace_only','\n'.join([' ', '\t', ''])), ] if__name__=='__main__': runner=pyperf.Runner() args=runner.parse_args() forname, textinsetup_cases(): runner.bench_func(nameor'default', textwrap_dedent, text)

@AA-Turner
Copy link
MemberAuthor

Thanks all!

A

@AA-TurnerAA-Turner enabled auto-merge (squash) March 31, 2025 00:12
@AA-TurnerAA-Turner changed the title gh-131791: Optimise textwrap.dedent()gh-130167: Optimise textwrap.dedent()Mar 31, 2025
@AA-TurnerAA-Turner disabled auto-merge March 31, 2025 00:13
@AA-TurnerAA-Turner enabled auto-merge (squash) March 31, 2025 00:14
@AA-TurnerAA-Turner merged commit 6aa88a2 into python:mainMar 31, 2025
39 checks passed
@AA-TurnerAA-Turner deleted the tw-dedent branch March 31, 2025 00:41
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 CentOS9 NoGIL Refleaks 3.x (tier-1) has failed when building commit 6aa88a2.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1610/builds/1118) and take a look at the build logs.
  4. Check if the failure is related to this commit (6aa88a2) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/1610/builds/1118

Failed tests:

  • test_free_threading

Summary of the results of the build (if available):

==

Click to see traceback logs
remote: Enumerating objects: 20, done. remote: Counting objects: 5% (1/20) remote: Counting objects: 10% (2/20) remote: Counting objects: 15% (3/20) remote: Counting objects: 20% (4/20) remote: Counting objects: 25% (5/20) remote: Counting objects: 30% (6/20) remote: Counting objects: 35% (7/20) remote: Counting objects: 40% (8/20) remote: Counting objects: 45% (9/20) remote: Counting objects: 50% (10/20) remote: Counting objects: 55% (11/20) remote: Counting objects: 60% (12/20) remote: Counting objects: 65% (13/20) remote: Counting objects: 70% (14/20) remote: Counting objects: 75% (15/20) remote: Counting objects: 80% (16/20) remote: Counting objects: 85% (17/20) remote: Counting objects: 90% (18/20) remote: Counting objects: 95% (19/20) remote: Counting objects: 100% (20/20) remote: Counting objects: 100% (20/20), done. remote: Compressing objects: 9% (1/11) remote: Compressing objects: 18% (2/11) remote: Compressing objects: 27% (3/11) remote: Compressing objects: 36% (4/11) remote: Compressing objects: 45% (5/11) remote: Compressing objects: 54% (6/11) remote: Compressing objects: 63% (7/11) remote: Compressing objects: 72% (8/11) remote: Compressing objects: 81% (9/11) remote: Compressing objects: 90% (10/11) remote: Compressing objects: 100% (11/11) remote: Compressing objects: 100% (11/11), done. remote: Total 11 (delta 9), reused 0 (delta 0), pack-reused 0 (from 0)  From https://github.com/python/cpython * branch main -> FETCH_HEAD Note: switching to '6aa88a2cb36240fe2b587f2e82043873270a27cf'. You are in 'detached HEAD' state. You can look around, make experimental changes and commit them, and you can discard any commits you make in this state without impacting any branches by switching back to a branch. If you want to create a new branch to retain commits you create, you may do so (now or later) by using -c with the switch command. Example: git switch -c <new-branch-name> Or undo this operation with: git switch - Turn off this advice by setting config variable advice.detachedHead to false HEAD is now at 6aa88a2cb36 gh-130167: Optimise ``textwrap.dedent()`` (#131919) Switched to and reset branch 'main' configure: WARNING: no system libmpdecimal found; falling back to bundled libmpdecimal (deprecated and scheduled for removal in Python 3.15)make: *** [Makefile:2334: buildbottest] Error 2

@hroncok
Copy link
Contributor

This changes the exception type when bad type is passed:

Before:

>>> import textwrap >>> textwrap.dedent(5) Traceback (most recent call last): File "<python-input-1>", line 1, in <module> textwrap.dedent(5) ~~~~~~~~~~~~~~~^^^ File "/usr/lib64/python3.14/textwrap.py", line 435, in dedent text = _whitespace_only_re.sub('', text) TypeError: expected string or bytes-like object, got 'int'

After:

>>> import textwrap >>> textwrap.dedent(5) Traceback (most recent call last): File "<python-input-1>", line 1, in <module> textwrap.dedent(5) ~~~~~~~~~~~~~~~^^^ File "/usr/lib64/python3.14/textwrap.py", line 432, in dedent lines = text.split('\n') ^^^^^^^^^^ AttributeError: 'int' object has no attribute 'split'

The exception context itself could be better in both cases, but arguably, TypeError is better here. This breaks tests assumption in https://github.com/laurent-laporte-pro/deprecated/blob/3f67ca71143479ef7ce71786538b2bc0ef9ee4b4/tests/test_deprecated.py#L201

(Originally from #107369 (comment))

@picnixz
Copy link
Member

Oh that's indeed an oversight. TypeError is a better error so let's wrap the call and reraise a TypeError instead if we catch an AttributeError

cc @AA-Turner

@AA-Turner
Copy link
MemberAuthor

I'll prepare a PR.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@AA-Turner@picnixz@eendebakpt@Marius-Juston@bedevere-bot@hroncok