Optimize raw HTML post-processor #1510

pawamoy · 2025-02-21T15:34:10Z

TODO

write a second batch of tests for cases where the list contains duplicates
~~maybe return new instances of _BlockLevelElements instead of list/set in relevant methods~~
~~maybe use stacklevel=2~~
~~maybe use warnings.deprecated (and typing_extensions.deprecated on Python 3.12-)~~ use existing deprecated function instead of warnings.warn

pawamoy · 2025-02-21T15:53:35Z

Hmm, the list->set change could be seen as breaking. We can instead create a new Markdown._block_level_elements attribute, and use that in isblocklevel(). Let me know if you think that's best.

markdown/postprocessors.py

waylan

This looks good generally. However, I haven't tested it or done any performance comparisons of my own. From a quick reading of the code only the few things you pointed out stand out to me. I'll need to take a closer look when I have more time.

markdown/postprocessors.py

facelessuser · 2025-02-26T16:53:07Z

I'll likely try this branch on some projects to inspect for regressions, just to be sure

waylan · 2025-02-26T16:59:57Z

Hmm, the list->set change could be seen as breaking. We can instead create a new Markdown._block_level_elements attribute, and use that in isblocklevel(). Let me know if you think that's best.

This is a valid concern. I'm not sure what to think about it.

facelessuser · 2025-02-26T17:09:43Z

Yes, I saw the change of blocks using a list to set and meant to comment on it. I'm pretty sure it'll break some of my plugins, so keeping the original but deprecating it in favor of a new structure is probably the way to go.

pawamoy · 2025-02-26T17:19:22Z

Thanks for confirming @facelessuser! I'll change it.

pawamoy · 2025-02-26T17:27:30Z

I just added a new private _block_level_elements attribute for now, but we should probably deprecate the old one and make the new one public. I'll wait for your input on this @waylan.

markdown/core.py

Using a set allows for better performances when checking for membership of a tag within block level elements. Issue-1507: Python-Markdown#1507

Previously, the raw HTML post-processor would precompute all possible replacements for placeholders in a string, based on the HTML stash. It would then apply a regular expression substitution using these replacements. Finally, if the text changed, it would recurse, and do all that again. This was inefficient because placeholders were re-computed each time it recursed, and because only a few replacements would be used anyway. This change moves the recursion into the regular expression substitution, so that: 1. the regular expression does minimal work on the text (contrary to re-scanning text already scanned in previous frames); 2. but more importantly, replacements aren't computed ahead of time anymore (and even less *several times*), and only fetched from the HTML stash as placeholders are found in the text. The substitution function relies on the regular expression groups ordering: we make sure to match `<p>PLACEHOLDER</p>` first, before `PLACEHOLDER`. The presence of a wrapping `p` tag indicates whether to wrap again the substitution result, or not (also depending on whether the substituted HTML is a block-level tag). Issue-1507: Python-Markdown#1507

markdown/util.py

waylan · 2025-02-27T20:30:00Z

markdown/util.py

-BLOCK_LEVEL_ELEMENTS: list[str] = [
+# TODO: Raise errors from list methods in the future.
+# Later, remove this class entirely and use a regular set.
+class _BlockLevelElements:


Elsewhere the type is specified as set. However, as this is not a subclass of set, that would be incorrect. Also, later when we remove this, then we will have a set. Perhaps this class should properly reflect that now. Unfortunately, there are no specialized container datatypes for sets in collections. And sets are not a Sequence Type either. Not really sure how best to address this.

My feeling was that it was OK to lie about the type, so that users actually get type warnings when they use it as a list instead of a set. That's one more (very effective) way of communicating the change. And since we implement all set functionality, the lie is not too big 🙂

We could do that, but some users run type validation on their code and this could result in bug reports. Either we need to fully document our reasoning for the inconsistency in the code comments or we need to make a change to the code.

Users running type checkers on their code is exactly what I expect, yes 😄 So, my opinion is, yes, lets keep lying about the type, and document that prominently (both with code comments and release/changelog notes).

Type correctness would otherwise mean that:

we type the variables as _BlockLevelElements

which in turn means we'd have to document this class properly

which in turn (probably) means we'd have to make it public

(also we'd likely want to use Self from typing_extensions as return annotation of methods retuning self, which means an additional dependency depending on the Python version)

I know it's far from being exhaustive but a search on GitHub returns only one result for use of md.block_level_elements (apart from @facelessuser's pymdownx which creates a new set from it, and discarding forks and venvs), and it's using remove, which is both a list and set method. That's IMO a strong indicator that md.block_level_elements has very few uses in the wild, and so immediately advertising its type as set should have a very small impact. I could be wrong though 🤷

It's not just type validators that I'm thinking about here. What about someone who is using isinstance (or similar) on the object in their code? They could reasonably expect isinstance(set) to return True, which is does not. For that matter existing code could reasonable expect isisntance(list) to return True. My objection is that neither return True. However, if _BlockLevelElements was a subclass of something, then it could return True for one of either set or list. That's why I pointed out above that set is not a Sequence Type. I was hoping it was and then _BlockLevelElements could just subclass the relevant baseclass and return True for both. I think we need to return True for at least one. The question is: which one? Using set is a breaking change but what it will end up being when the transition is complete. Using list avoids the breaking change, but it not forward looking. And I don't think there is any way to raise a deprecation warning if a user calls isinstance on the objection.

My point is that I think _BlockLevelElements needs to be either a set or a list at a minimum. Ideally it would be both, but that is probably not sensible. Any solution is going to be a compromise. I'm saying that having _BlockLevelElements not be either a set or a list is not the compromise I want to make.

Good point. I think we could inherit from both list and set since we implement all their methods anyway? This way isinstance would return True for both. Also, we can customize the behavior of isinstance thanks to __instancecheck__ to emit a deprecation warning when checking against list 🙂

OK __instancecheck__ works the other way unfortunately, it's not meant for this use-case. Inheriting from set breaks a lot of tests (121 of them), not sure exactly why. Inheriting from list works, but as you pointed, isinstance(ref, set) will return False.

pawamoy

First batch of tests added. It's pretty basic. A second batch should be added to test lists with duplicate elements within them. I updated the code to try and reduce the possibility of duplicates but it's not thorough.

markdown/util.py

markdown/extensions/md_in_html.py

tests/test_block_level_elements.py

pawamoy · 2025-03-01T12:40:53Z

About duplicates: I don't prevent their insertion, otherwise we lose backwards compatibility again. We'll just have to add test cases with duplicates 🙂

A few other remarks/suggestions:

we could use warnings.deprecated to decorate deprecated methods (instead of calling warnings.warn ourselves), so that type checkers will warn about their use, and IDEs will strike them through (visual indication of deprecation). For Python less than 3.13 we'd have to depend on typing-extensions.
in any case, we could pass stacklevel=2 to warnings, so that they show the user line calling these deprecated methods.
I've developed a small tool called Yore that aims at helping manage legacy code. Instead of adding various # TODO comments throughout the code, you instead add # YORE structured comments and can run yore check to check if legacy code should be removed/transformed (Python version just reached its end of life / you're ready to bump the major version) and yore fix to automatically apply prepared transforms. Just sharing, can help setting it up if you're interested 😄 (it's just a matter of adding the dev-dep and maybe calling it in tox/CI).

pawamoy · 2025-03-06T16:11:18Z

I added tests with duplicates.

pawamoy · 2025-03-06T16:23:17Z

markdown/util.py

+        # We raise `ValueError` for backwards compatibility.
+        try:
+            self._set.remove(element)
+        except KeyError:
+            raise ValueError(f"{element!r} not in list") from None


Not sure about this. It's difficult to deprecate a raised exception in favor of another one. Maybe we should immediately start raising KeyError.

About deprecating raised exceptions: we could create new exception that inherits from both ValueError and KeyError, so that both can be caught in user code. I was hoping we could hook into the except mechanism to only emit a warning when ValueError or KeyError is caught, but it's not possible unfortunately, see https://discuss.python.org/t/instance-subclass-check-for-except-clauses/71583. So we're stuck with either:

not emitting any warning

always emitting a warning, that users will have to ignore even if they catch the right exception

I'm inclined to suggest that we not bother ourselves with the errors here. We can just call _list.remove and _set.remove and allow any errors to bubble up to the calling code. Is there any reason why we shouldn't do that?

Agree on keeping things simple. I'll try a different approach (we still want to remove all matching items from the list, not just one) 🤔

waylan · 2025-03-12T15:56:24Z

Sorry, but I was out for a couple of weeks. Back now and trying to catch up.

we could use warnings.deprecated to decorate deprecated methods (instead of calling warnings.warn ourselves

Yes, let's do that. I would have mentioned this earlier but missed that we weren't using warn.deprecated already.

we could pass stacklevel=2 to warnings

We have not traditionally done this in the past. I have always taken the position that we are not responsible for Python's default behavior. As Python by default hides deprecation warnings, then users should expect that and act accordingly. But I'm not opposed to this either.

I've developed a small tool called Yore that aims at helping manage legacy code. Instead of adding various # TODO comments throughout the code, you instead add # YORE structured comments and can run yore check

It is easy enough to search for #TODO. Adding dependencies just creates more complexity.

pawamoy · 2025-03-12T19:14:56Z

Sorry, but I was out for a couple of weeks. Back now and trying to catch up.

Absolutely no worries, and no need to apologize, thank you very much for your answers 🙂 I'll address the various points you've answered.

pawamoy · 2025-03-13T13:23:52Z

Oh, I noticed again that you have your own deprecated implementation. I'll use it instead of warnings.deprecated or typing_extensions.deprecated under 3.13, and we can later remove this custom implementation in favor of the official one, to reduce the amount of changes in this PR. I'll just have to move the function definition up, since we'll have to use it at the module-level.

pawamoy · 2025-03-13T14:00:57Z

markdown/util.py

+    def pop(self, index: int | None = None, /) -> str:
+        # In-place pop should update both list and set.
+        if index is None:
+            index = -1
+        else:
+            warnings.warn(
+                "Using block level elements as a list is deprecated, use it as a set instead.",
+                DeprecationWarning,
+            )


We still use warnings.warn here because we don't want to deprecate the whole method, only the index parameter.

pawamoy force-pushed the optimize-rawhtml-postprocessor branch from 6113aad to fc9acc0 Compare February 21, 2025 15:38

This comment was marked as resolved.

Sign in to view

pawamoy commented Feb 22, 2025

View reviewed changes

markdown/postprocessors.py Outdated Show resolved Hide resolved

markdown/postprocessors.py Show resolved Hide resolved

pawamoy commented Feb 23, 2025

View reviewed changes

markdown/postprocessors.py Outdated Show resolved Hide resolved

waylan requested changes Feb 26, 2025

View reviewed changes

markdown/postprocessors.py Outdated Show resolved Hide resolved

markdown/postprocessors.py Outdated Show resolved Hide resolved

markdown/postprocessors.py Show resolved Hide resolved

waylan added needs-review Needs to be reviewed and/or approved. requires-changes Awaiting updates after a review. labels Feb 26, 2025

pawamoy force-pushed the optimize-rawhtml-postprocessor branch from ce408be to 0841396 Compare February 26, 2025 17:17

pawamoy force-pushed the optimize-rawhtml-postprocessor branch from 0841396 to e05cce7 Compare February 26, 2025 17:26

facelessuser reviewed Feb 26, 2025

View reviewed changes

markdown/core.py Outdated Show resolved Hide resolved

pawamoy added 3 commits February 26, 2025 21:20

Use set instead of list for block level elements

4334dee

Using a set allows for better performances when checking for membership of a tag within block level elements. Issue-1507: Python-Markdown#1507

Add changelog entry for improved raw HTML post-processor perfs

1bc8c54

pawamoy force-pushed the optimize-rawhtml-postprocessor branch from e05cce7 to 1bc8c54 Compare February 26, 2025 20:20

fixup! Use set instead of list for block level elements

d4813dc

pawamoy commented Feb 27, 2025

View reviewed changes

markdown/util.py Outdated Show resolved Hide resolved

markdown/util.py Show resolved Hide resolved

markdown/util.py Outdated Show resolved Hide resolved

markdown/util.py Show resolved Hide resolved

waylan reviewed Feb 27, 2025

View reviewed changes

fixup! Use set instead of list for block level elements

6a4bfcb

pawamoy commented Feb 28, 2025

View reviewed changes

markdown/util.py Outdated Show resolved Hide resolved

pawamoy commented Feb 28, 2025

View reviewed changes

markdown/util.py Outdated Show resolved Hide resolved

pawamoy commented Feb 28, 2025

View reviewed changes

markdown/util.py Outdated Show resolved Hide resolved

waylan reviewed Feb 28, 2025

View reviewed changes

markdown/extensions/md_in_html.py Outdated Show resolved Hide resolved

waylan reviewed Feb 28, 2025

View reviewed changes

tests/test_block_level_elements.py Show resolved Hide resolved

pawamoy added 4 commits March 1, 2025 11:52

fixup! Use set instead of list for block level elements

5856824

fixup! Use set instead of list for block level elements

dd759eb

fixup! Use set instead of list for block level elements

fcc2494

fixup! Use set instead of list for block level elements

60c5355

pawamoy added 2 commits March 6, 2025 17:09

fixup! Use set instead of list for block level elements

adca84b

fixup! Use set instead of list for block level elements

d2aaef0

pawamoy added 2 commits March 6, 2025 17:14

fixup! Use set instead of list for block level elements

86ed890

fixup! Use set instead of list for block level elements

ff23488

pawamoy commented Mar 6, 2025

View reviewed changes

pawamoy mentioned this pull request Mar 10, 2025

feature: Back links! mkdocstrings/mkdocstrings#723

Closed

fixup! Use set instead of list for block level elements

180c476

pawamoy commented Mar 13, 2025

View reviewed changes

fixup! Use set instead of list for block level elements

0fbc19d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize raw HTML post-processor #1510

Optimize raw HTML post-processor #1510

pawamoy commented Feb 21, 2025 •

edited

Loading

pawamoy commented Feb 21, 2025

This comment was marked as resolved.

waylan left a comment

facelessuser commented Feb 26, 2025

waylan commented Feb 26, 2025

facelessuser commented Feb 26, 2025

pawamoy commented Feb 26, 2025

pawamoy commented Feb 26, 2025 •

edited

Loading

waylan Feb 27, 2025

pawamoy Feb 27, 2025 •

edited

Loading

waylan Feb 28, 2025

pawamoy Mar 1, 2025

waylan Mar 12, 2025

pawamoy Mar 12, 2025 •

edited

Loading

pawamoy Mar 13, 2025

pawamoy left a comment

pawamoy commented Mar 1, 2025 •

edited

Loading

pawamoy commented Mar 6, 2025

pawamoy Mar 6, 2025

pawamoy Mar 12, 2025 •

edited

Loading

waylan Mar 12, 2025

pawamoy Mar 12, 2025

waylan commented Mar 12, 2025

pawamoy commented Mar 12, 2025

pawamoy commented Mar 13, 2025 •

edited

Loading

pawamoy Mar 13, 2025

Optimize raw HTML post-processor #1510

Are you sure you want to change the base?

Optimize raw HTML post-processor #1510

Conversation

pawamoy commented Feb 21, 2025 • edited Loading

pawamoy commented Feb 21, 2025

This comment was marked as resolved.

waylan left a comment

Choose a reason for hiding this comment

facelessuser commented Feb 26, 2025

waylan commented Feb 26, 2025

facelessuser commented Feb 26, 2025

pawamoy commented Feb 26, 2025

pawamoy commented Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

pawamoy Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawamoy Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawamoy left a comment

Choose a reason for hiding this comment

pawamoy commented Mar 1, 2025 • edited Loading

pawamoy commented Mar 6, 2025

Choose a reason for hiding this comment

pawamoy Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waylan commented Mar 12, 2025

pawamoy commented Mar 12, 2025

pawamoy commented Mar 13, 2025 • edited Loading

Choose a reason for hiding this comment

pawamoy commented Feb 21, 2025 •

edited

Loading

pawamoy commented Feb 26, 2025 •

edited

Loading

pawamoy Feb 27, 2025 •

edited

Loading

pawamoy Mar 12, 2025 •

edited

Loading

pawamoy commented Mar 1, 2025 •

edited

Loading

pawamoy Mar 12, 2025 •

edited

Loading

pawamoy commented Mar 13, 2025 •

edited

Loading