Skip to content

perf: speed up _unescape_special_chars for large documents #748

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nicholasserra merged 6 commits into from
Nov 9, 2026
Merged

perf: speed up _unescape_special_chars for large documents #748

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fb5899e
perf: speed up _unescape_special_chars for large documents
SwastikTripathi Oct 25, 2026
4bf3ddb
Address review: nonlocal flag, define _sub once, ASCII comments
SwastikTripathi Oct 27, 2026
ed6330a
Merge remote-tracking branch 'origin/master' into md2-748-merge
SwastikTripathi Oct 27, 2026
58cd6cb
Merge remote-tracking branch 'origin/master' into perf/unescape-speci…
SwastikTripathi Nov 7, 2026
be9bb4e
Simplify the unescape loop to while replaced
SwastikTripathi Nov 7, 2026
a13e621
Merge branch 'master' into perf/unescape-special-chars
nicholasserra Nov 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 21 additions & 10 deletions lib/markdown2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2377,18 +2377,29 @@ def _encode_email_address(self, addr: str) -> str:

def _unescape_special_chars(self, text: str) -> str:
# Swap back in all the special characters we've hidden.
hashmap = tuple(self._escape_table.items()) + tuple(self._code_table.items())
# html_blocks table is in format {hash: item} compared to usual {item: hash}
hashmap += tuple(tuple(reversed(i)) for i in self.html_blocks.items())
replacements = {}
for ch, hash in hashmap:
if 'md5-' not in text:
return text
# Build hash→original mapping directly without intermediate tuple.

@Crozzers Crozzers Oct 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:
Image
As opposed to this which is bigger and more legible:
Image

@SwastikTripathi SwastikTripathi Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 4bf3ddb, it's plain -> now.

# html_blocks is stored inverted ({hash: item}) compared to the others.
replacements: dict[str, str] = {}
for ch, hash in self._escape_table.items():

@Crozzers Crozzers Nov 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it be perhaps faster and simpler if instead of building a replacements dict, you just did this in the _sub function:

val = (
    self._escape_table.get(match[0])
    or self._code_table.get(match[0])
    or self.html_blocks.get(match[0])
)

Maybe more memory efficient?

@SwastikTripathi SwastikTripathi Nov 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That works for html_blocks, which is keyed by hash, but _escape_table and _code_table go the other way ({original: hash}, e.g. self._code_table[text] = hashed), so .get(match[0]) on them would always miss. To skip the dict build we would need reverse tables kept in sync at every place those two are written (the escape table is also written per URL in _protect_url). The early md5- check already skips the build for most calls, so I kept the dict here. Happy to add the reverse tables if you prefer that.

@Crozzers Crozzers Nov 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah yes, forgot about that. In that case building the map is probably the best way

replacements.setdefault(hash, ch)
for ch, hash in self._code_table.items():
replacements.setdefault(hash, ch)
for hash, item in self.html_blocks.items():
replacements.setdefault(hash, item)
# Scan once per nesting level. Use a flag instead of a full-string
# comparison to detect whether any replacement was made this pass.
while True:
orig_text = text
# Scan once per nesting level instead of once for every stored hash.
text = re.sub(r'md5-[0-9a-f]{32}',
lambda match: replacements.get(match[0], match[0]), text)
if text == orig_text:
replaced = [False]

@Crozzers Crozzers Oct 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interesting but odd pattern. Would nonlocal work here? Bit cleaner?

@SwastikTripathi SwastikTripathi Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, switched to nonlocal in 4bf3ddb.

def _sub(match: re.Match, _rep: dict = replacements) -> str:

@Crozzers Crozzers Oct 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does _rep need defining as a kwarg with a default value? Can the function not just use replacements directly?

@Crozzers Crozzers Oct 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also _sub is being re-defined every loop?

@SwastikTripathi SwastikTripathi Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, it doesn't. It uses replacements directly now (4bf3ddb).

@SwastikTripathi SwastikTripathi Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch. It's defined once before the loop now, and only the flag is reset each pass (4bf3ddb).

val = _rep.get(match[0])
if val is not None:
replaced[0] = True
return val
return match[0]
text = re.sub(r'md5-[0-9a-f]{32}', _sub, text)
if not replaced[0]:
break
return text

Expand Down
Loading