Skip to content

Improve formatting performance for subscript chains - #5275

Closed
happykawayigt wants to merge 2 commits into
psf:mainfrom
happykawayigt:perf/subscript-chain-splitting
Closed

happykawayigt wants to merge 2 commits into
psf:mainfrom
happykawayigt:perf/subscript-chain-splitting

Conversation

@happykawayigt

@happykawayigt happykawayigt commented Jul 31, 2026 •

Copy link
Copy Markdown

Description

Fixes #5270.
Closes #5475
Closes #5305

Long chains of single-token subscript trailers repeatedly rebuild the same prefix Line and BracketTracker state during recursive right-hand splitting. This patch detects that narrowly defined no-comment shape, reuses the already validated leaf metadata, caches cumulative ASCII widths for prefix heads, and falls back to the existing path for complex subscripts, comments, non-ASCII text, or other bracket shapes.

The formatting result is unchanged. In local CPython 3.12 benchmarks for x = a[0][1]... at the default line length:

Subscripts Before After
80 1.48 s 0.25 s
160 11.08 s 0.80 s
320 45.43 s 1.80 s

Output SHA-256 matched the base branch at every benchmark size. A 135-case matrix covering numeric, name, and string subscripts across multiple line lengths and contexts also matched byte-for-byte.

Validation:

  • pytest tests/test_format.py -q (230 passed)
  • pytest tests/test_black.py -q -k "not symlink" (160 passed, 6 deselected, 8 subtests passed)
  • pre-commit on changed files: isort, flake8, mypy, EOF, trailing whitespace passed
  • git diff --check
  • New regression test bounds repeated BracketTracker.mark() calls and verifies idempotence

The three excluded test_black.py cases require Windows symlink privileges and fail locally with WinError 1314; they are unrelated to this change.

AI assistance was used during implementation. I independently reproduced the issue, profiled the split path, reviewed the final diff, compared outputs against an untouched base worktree, and ran the validation above.

Checklist - did you ...

  • Implement any code style changes under the --preview style, following the stability policy? (No style change.)
  • Add an entry in CHANGES.md if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation? (No documentation change is needed.)

@happykawayigt
happykawayigt marked this pull request as ready for review August 1, 2026 18:24
@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

diff-shades results comparing this PR (5c1e307) to main (c7801d9):

--preview style: no changes

--stable style: no changes


What is this? | Workflow run | diff-shades documentation

Comment thread tests/test_black.py
with patch.object(BracketTracker, "mark", counting_mark):
formatted = black.format_str(source, mode=Mode())

assert len(formatted.splitlines()) == 233

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This exact number could get confusing to update if formatting changes.
Not sure that it even makes sense to check the formatting, the test is of the performance, not of the formatting.

@cobaltt7

cobaltt7 commented Oct 3, 2026

Copy link
Copy Markdown
Collaborator

Could you add comments throughout this code? This is a fairly large code change and I find it difficult to follow everywhere - particularly the changes inside bracket_split_build_line().

Sign up for free to 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.

Quadratic formatting time for chains of subscripts (7s for one 160-subscript line; attribute/call chains stay linear)

2 participants