avoid quadratic per-leaf copy in StringMerger.do_transform#5220
Open
sahvx655-wq wants to merge 1 commit into
Open
avoid quadratic per-leaf copy in StringMerger.do_transform#5220sahvx655-wq wants to merge 1 commit into
sahvx655-wq wants to merge 1 commit into
Conversation
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
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.
Description
Profiling
--previewstring merging on"%s ..." % (a, b, c, ...)still showed a quadratic after #5199 linearisedappend_leaves: the run sat around 4s for 8000 operands withlencalled about 32 million times.StringMerger.do_transformrebuilds the merged line by walking every leaf of the old line and copying each leaf that is not part of a merged string with its ownappend_leaves(new_line, line, [leaf])call. The resume hint added in #5199 only persists inside a singleappend_leavescall, so a one-leaf call always restarts the search for the leaf's position from the front. A%expression's operand tuple is a single parent holding every operand, so copying its n leaves one at a time rescans that child list from index 0 each time and is O(n²). It is reachable pre-auth fromblackdwithX-PreviewandX-Enable-Unstable-Feature: string_processing.I gather the consecutive leaves that fall outside any merged string group and hand each contiguous run to
append_leavesin one call, which is the shape the resume hint was built for, so the copy stays linear. The per-leaf calls were doing nothing the batched call does not, comments included, so the new line is built identically. Output is byte-identical across the test suite and the black source tree in stable, preview and unstable modes plus crafted%-format and implicit-concat inputs; the 8000-operand case drops from roughly 4.1s to 0.8s and the curve flattens from quadratic to linear.Checklist - did you ...
--previewstyle, following the stability policy?CHANGES.mdif necessary?