Replace auto-linked URLs in a single pass - #41
Open
arpitjain099 wants to merge 1 commit into
Open
Conversation
Both halves of the link replacement in _transform rebuilt the whole string once per URL: the search loop spliced a token in with a slice concatenation for every match, and the restore step ran a separate str.replace for every token. A post with many links therefore cost time quadratic in its length. Use re.sub for both, so each is one linear pass. Output is unchanged. On 448 KB of text made of 32,000 short links, render_html goes from 22.2s to 0.071s, and timings now double with the input rather than quadrupling. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
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.
This is the patch you offered to look at.
_transformdid the link replacement in two halves and both were quadratic. The search loop pulled one match at a time and spliced the token in withdata = data[:start] + token + data[end:], rebuilding the whole string for every URL, and the restore step at the end ran a separatedata.replace(token, replacement)for every token, each of which scans the whole string again. Neither is noticeable with a handful of links, but a long post made of many short ones pays O(n^2).Both are now a single
re.sub. The token format is unchanged, so it still survives the escape and cosmetic passes the same way, and_link_replaceis called from the callback exactly as before.Timings through
render_htmlon"http://a.co/x "repeated, Python 3.12:The shape matters more than the ratio: the new column doubles as the input doubles, the old one roughly quadruples.
On not changing behaviour, I ran the old and new modules side by side over 4,008 generated inputs across 4 option combinations, 16,032 comparisons, and the output is byte identical. The generator included the awkward cases I could think of: text that already looks like a placeholder token, links in parentheses, links followed by punctuation,
--and...next to links,[url]tags, andreplace_links=False.Two things worth flagging rather than hiding.
The two tests I added are not regression tests for the slowness, and they pass on both the old and new code. They cover the behaviour this refactor touches, many links all getting replaced and a token-looking string being left alone, so a future change to this area has something to trip over. I left out a timing assertion deliberately since those go flaky in CI.
There is one behavioural nuance. The old restore loop ran
str.replaceper token in sequence, so each pass could see the text produced by the previous one. The singlere.subdoes not re-scan its own output. I could not construct an input where that differs, because_url_recannot match a token, but a customlinkerthat returned text shaped like{{ bbcode-link-N }}would behave differently. That seems like the better behaviour to me, though it is your call.