The attached patch avoid to build the whole list of visitable nodes when only the first is needed.
I've observed a two order of magnitude speed improvement with sphinx-build and large documents.
The same time-saving should be achieved without additional argument when Node.traverse()
returns an iterator instead of list.
The docstring specifies the return value as "iterable" so this change should (in principle) not
affect the API.
However, there are some places in Docutils code where the return value of Node.traverse() is assumed
to be a list. Revision [r8387] fixes this.
Sphinx in at least two places expects a list on return, then your change will lead to troubles in dependents that abuse the documented API. What is the best way to cope with that?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I suspectes something like this. The new patch keeps Node.traverse() returning a list while using the iterator returned by Node._traverse() for Node.next_node().
This should give the time improvement in a backwards compatible way.
In addition, we can announce an upcoming change to let Node.traverse() return an iterator in a future release giving 3rd party programmers time to make their code failsafe.
To get the best from this last patch you should change most occurrences of Node.traverse() to Node._traverse() (and possibly remove recently added list() wrapping to the others).
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
The latest patch is a proof of concept: if it achieves the dramatic speed improvement, we must consider how to best implement this change in a clean way.
If Node.traverse() is going to return an iterator (instead of a list) in future, it may be better not to change calls to use _traverse() instead for just two versions.
If Node.traverse() will keep returning a list, we may use a name like xtraverse() for the iterator-returning variant (and use this exclusively in Docutils code).
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Thank you for the patch.
The same time-saving should be achieved without additional argument when Node.traverse()
returns an iterator instead of list.
The docstring specifies the return value as "iterable" so this change should (in principle) not
affect the API.
However, there are some places in Docutils code where the return value of Node.traverse() is assumed
to be a list. Revision [r8387] fixes this.
Could you test the attached patch?
Related
Commit: [r8387]
Last edit: Günter Milde 2019-09-07
Diff:
Sphinx in at least two places expects a list on return, then your change will lead to troubles in dependents that abuse the documented API. What is the best way to cope with that?
I suspectes something like this. The new patch keeps Node.traverse() returning a list while using the iterator returned by Node._traverse() for Node.next_node().
This should give the time improvement in a backwards compatible way.
In addition, we can announce an upcoming change to let Node.traverse() return an iterator in a future release giving 3rd party programmers time to make their code failsafe.
To get the best from this last patch you should change most occurrences of Node.traverse() to Node._traverse() (and possibly remove recently added list() wrapping to the others).
The latest patch is a proof of concept: if it achieves the dramatic speed improvement, we must consider how to best implement this change in a clean way.
If Node.traverse() is going to return an iterator (instead of a list) in future, it may be better not to change calls to use _traverse() instead for just two versions.
If Node.traverse() will keep returning a list, we may use a name like xtraverse() for the iterator-returning variant (and use this exclusively in Docutils code).
I confirm that your patch gives results similar to the ones achieved by original patch.
Thank you for confirming the speed improvement and thanks again for reporting the issue.
Fixed in [r8389] .
Related
Commit: [r8389]