Prune the new node in _resolve_toctree instead of copying, to accelerate building
#14278
Conversation
Ivorforce
force-pushed
the
prune-dont-copy
branch
from
164997c to
f9d48c0
Compare
| ) | ||
| return | ||
|
|
||
| if isinstance(node, (nodes.reference, nodes.title)): |
There was a problem hiding this comment.
nodes.reference may be quite common so let us have the check early.
| ): | ||
| node: Element | ||
|
|
||
| if isinstance(node, (addnodes.compact_paragraph, nodes.list_item)): |
There was a problem hiding this comment.
Can you reorder all isintance checks from the more likely to the less likely chrck and prioritize early exits?
| return | ||
|
|
||
| msg = f'Unexpected node type {node.__class__.__name__!r}!' | ||
| raise ValueError(msg) |
There was a problem hiding this comment.
Let us raise a TypeError instead.
| _toctree_prune_seq(node, depth, maxdepth, collapse, tags, initial_call=True) | ||
|
|
||
|
|
||
| def _toctree_prune_seq( |
There was a problem hiding this comment.
Why not using a node transformer with a visitor pattern? (though... it could have surprising effects for subclasses as I am not sure the visitor is polymorphic)
|
Cutting of children may result in a bad tree at the end with some information possibly lost (e.g. the event handlers may have surprises) |
|
@picnixz Thank you for the review!
Do you think there's a way to address this without making a copy of the entire tree? |
|
Unfortunately no. The simple fact that the current tests fail is also an indication that this changed the behavior :/ |
|
Am i understanding this right — there's no way to remove children from |
|
I need to check this again as I do not remember exactly what state is assumed here. However, any tree modifications that drop children may not necessarily be possible. It is fine to make a copy as this becomes a transformed tree but an in-place transformation may not be preferrable (maybe changing the tests is also correct but we need to be careful of whether the tree is getting passed to event handlers at some point of the resolution) |
|
Looking at the code again, I think I may just have mistransformed the return values for the in-place pruning. I'll try again. |
Ivorforce
force-pushed
the
prune-dont-copy
branch
from
f9d48c0 to
12ac5d4
Compare
|
Looks like the <ul class="simple">
</ul>These aren't rendered in the browser, so this has no visual effect. I'm not sure where that's coming from, as the |
2 participants
sphinx-buildis too slow in large projects #14277This accelerates
_resolve_toctree. In Godot's documentation, this saves ~15m of ~60m (25%).New profile results after optimizing:
results.prof.zip
Explanation
As found in #14277, the function
_resolve_toctreeappears to use_toctree_copyto prune it. The original is discarded, wasting CPU time.This PR changes the operation to prune in-place.
I've tested this with https://github.com/godotengine/godot-docs, and at a glance can find no regressions. However, since I'm not at all familiar with the codebase, this definitely needs an expert review :)