Repository navigation
perf: speed up _unescape_special_chars for large documents - #748
SwastikTripathi wants to merge 3 commits into
Conversation
Two O(n) costs were paid per pass through the while loop: 1. The termination check compared the full text string (`text == orig_text`), which is O(n) even when nothing changed. 2. The replacements dict was built from three intermediate tuples before every call (escape_table + code_table + reversed html_blocks). Fix both: - Skip immediately if 'md5-' is not present in the text (the common case for plain paragraphs with no inline code, links, or escapes). - Build the replacements dict by iterating the source dicts directly, without allocating intermediate tuples. - Replace the string comparison with a boolean flag set inside the re.sub callback; the loop breaks as soon as a full pass produces no replacements. Fixes trentm#635 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Crozzers
left a comment
Crozzers
left a comment
There was a problem hiding this comment.
Do you have any perf data that demonstrates an improvement? Any tests or profiling you've done, maybe a before and after?
| for ch, hash in hashmap: | ||
| if 'md5-' not in text: | ||
| return text | ||
| # Build hash→original mapping directly without intermediate tuple. |
There was a problem hiding this comment.
Nitpick but I would remove the → char in the comment in favour of plain text like ->.
I don't think it will cause any issues (python source is default utf-8) but I just think it can't be typed from a keyboard so should it be in a comment?
It also displays oddly in VSCode, really small and almost looks like an underline:
As opposed to this which is bigger and more legible:
| text = re.sub(r'md5-[0-9a-f]{32}', | ||
| lambda match: replacements.get(match[0], match[0]), text) | ||
| if text == orig_text: | ||
| replaced = [False] |
There was a problem hiding this comment.
Interesting but odd pattern. Would nonlocal work here? Bit cleaner?
| lambda match: replacements.get(match[0], match[0]), text) | ||
| if text == orig_text: | ||
| replaced = [False] | ||
| def _sub(match: re.Match, _rep: dict = replacements) -> str: |
There was a problem hiding this comment.
Does _rep need defining as a kwarg with a default value? Can the function not just use replacements directly?
There was a problem hiding this comment.
Also _sub is being re-defined every loop?
There was a problem hiding this comment.
No, it doesn't. It uses replacements directly now (4bf3ddb).
There was a problem hiding this comment.
Good catch. It's defined once before the loop now, and only the flag is reset each pass (4bf3ddb).
Use a nonlocal flag instead of a one-element list, define the re.sub callback once outside the loop, use replacements directly instead of a default kwarg, and replace the arrow character with plain ASCII. Add a CHANGES entry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the review. I hadn't measured it before, so I did now. Per-call cost, after converting a doc with 2000 code spans and 2000 HTML blocks (so
Most of the old cost was rebuilding the replacements dict on every call, which grows with the size of the tables, so on big documents the per-header and per-link calls get expensive. The early return skips that entirely when there's nothing to replace. End to end the gain is smaller than that suggests. On a synthetic 420 KB doc (2000 sections, each with a header, inline code, a link, an escape and inline HTML, extras Output is byte-identical between before and after on the benchmark docs (compared by sha1 of html + toc_html), and The inline nits are done in 4bf3ddb: |
# Conflicts: # CHANGES.md
Fixes #635.
Problem
_unescape_special_charshas two hidden O(n) costs that compound on large markdown documents:Full-string comparison for loop termination. Each pass through the
while Trueloop storesorig_text = textand then comparestext == orig_textto detect whether anything changed. On a large document this is an O(n) string comparison even when the very first regex scan found nothing to replace.Unnecessary intermediate allocations on every call. The replacements dict was built from three
tuple(...)calls, converting_escape_table.items(),_code_table.items(), and a generator expression overhtml_blocks.items(), before iterating them into the dict. That's three extra list/tuple objects per call.Fix
'md5-'is not present anywhere intext, return immediately. This is the common case for plain paragraphs that contain no inline code, links, backslash escapes, or raw HTML, skipping both the dict build and the regex engine entirely.replacedflag (a one-element list so the inner function can mutate it) is set inside there.subcallback whenever a match is found in the lookup table. The loop breaks when a full pass sets no flag, no full-string comparison needed.All existing tests pass (
test/test.py,test/api.doctests).🤖 Generated with Claude Code