perf(parser): make get_all_text() O(nodes) - #378
Merged
Conversation
rkfshakti
approved these changes
Jul 23, 2026
rkfshakti
left a comment
There was a problem hiding this comment.
The fix is clean and correct. Replacing the O(n^2) ancestor walk with a direct set lookup is the right approach — ignored_elements already contains all descendants via element.iter(), so a single owner not in ignored_elements check is sufficient.
The regression tests cover the key edge cases: comment tail text, deep nesting, and ignored subtrees. The depth=500 test gives confidence this won't regress on real-world pages.
One minor observation: the if element not in ignored_elements guard in the loop is redundant since set.update is idempotent, but it's harmless and makes the intent clearer.
Owner
|
Nice work as always @yetval |
Merged
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.
Problem
get_all_text()decides whether each text node is visible by walking up through all of its ancestors to see if any of them is inignore_tags. It does that walk once per text node, so the cost grows with both the number of text nodes and the depth of the tree (O(text_nodes x depth)). On deep pages this gets slow.Fix
We already know which elements are ignored, so instead of walking ancestors every time, I collect the ignored tags and all of their descendants into a set once up front. After that, checking a node is just a single set lookup, which makes the whole thing O(nodes).
The method still uses
.//text()under the hood, so comments, CDATA, and tail text are handled exactly the way they were before. Nothing about the API or the options changes, and every caller gets the speedup for free.Result
Output is byte for byte identical to the old version across 2240 combinations of HTML shapes, options, and
keep_commentssettings. The existing suite passes (139 tests), and I added regression tests for the comment-tail and deep-nesting cases.Timings on a deeply nested page:
The old timings climb faster than the tree grows, while the new ones scale linearly.