Improve Node#each_recursive performance
#139
Conversation
Add `benchmark/each_recursive.yaml` Rewrite `Node#each_recursive` implementation for performance Add a test for `Node#each_recursive`
| self.elements.each {|node| | ||
| block.call(node) | ||
| node.each_recursive(&block) | ||
| } | ||
| stack = [] | ||
| each { |child| stack.unshift child if child.node_type == :element } | ||
| until stack.empty? | ||
| child = stack.pop | ||
| yield child | ||
| n = stack.size | ||
| child.each { |grandchild| stack.insert n, grandchild if grandchild.node_type == :element } | ||
| end |
There was a problem hiding this comment.
Can we improve REXML::Elements#each(nil) case instead?
There was a problem hiding this comment.
This seems a possible improvement. But, IMHO, it can be split into another pull request.
There was a problem hiding this comment.
Do we still need to change REXML::Node#each_recursive when we improve REXML::Elements#each(nil)?
There was a problem hiding this comment.
This implementation is best so like that we don't need to change it when we improve something.
I don't really like implementations that are optimized when using specific arguments. From that point of view, I'm not very keen on optimizing each(nil).
There was a problem hiding this comment.
If elements.each(nil) is improved, we can use elements.each { |x| yield x } instead of each { |x| yield x if x.node_type == :element }.
However, I don't think it is a problem that there are some duplications of each { |x| yield x if x.node_type == :element } sometimes. Because elements.each(&) has some overheads and the performance is more important here.
Repeatedly, I talked about the performance. The maintainability and other matters are out of topic here because the speed of each_recursive is one of the crucial parts of the XML library.
There was a problem hiding this comment.
The maintainability and other matters are out of topic here
As a maintainer, I don't agree it.
But I can understand your opinion as a heavy each_recursive user.
OK. Let's focus on only each_recursive in this PR.
There was a problem hiding this comment.
Thank you for your understanding!
test/test_document.rb
Outdated
| 100.times do | ||
| x_node_source = '' | ||
| 100.times do |
There was a problem hiding this comment.
Do we need 100 for this test?
Can we reduce it?
Can we use different number for outer times and inner times for easy to distinct a child problem and a descendant problem.
test/test_document.rb
Outdated
| document.each_recursive { count += 1 } | ||
| # Node#each_recursive iterates elements only. | ||
| # This does not iterate XML declerations, comments, attributes, CDATA sections, etc. | ||
| assert_equal(100 * 100 + 1, count) |
There was a problem hiding this comment.
Can we use all collected element name instead of just the number of loop count?
Count only check will be difficult to debug when this is failed.
Use the small iteration numbers for test. Check names instead of counting elements. ruby#139 (comment) ruby#139 (comment)
| self.elements.each {|node| | ||
| block.call(node) | ||
| node.each_recursive(&block) | ||
| } | ||
| stack = [] | ||
| each { |child| stack.unshift child if child.node_type == :element } | ||
| until stack.empty? | ||
| child = stack.pop | ||
| yield child | ||
| n = stack.size | ||
| child.each { |grandchild| stack.insert n, grandchild if grandchild.node_type == :element } | ||
| end |
There was a problem hiding this comment.
Do we still need to change REXML::Node#each_recursive when we improve REXML::Elements#each(nil)?
Co-authored-by: Sutou Kouhei <[email protected]>
| self.elements.each {|node| | ||
| block.call(node) | ||
| node.each_recursive(&block) | ||
| } | ||
| stack = [] | ||
| each { |child| stack.unshift child if child.node_type == :element } | ||
| until stack.empty? | ||
| child = stack.pop | ||
| yield child | ||
| n = stack.size | ||
| child.each { |grandchild| stack.insert n, grandchild if grandchild.node_type == :element } | ||
| end |
There was a problem hiding this comment.
The maintainability and other matters are out of topic here
As a maintainer, I don't agree it.
But I can understand your opinion as a heavy each_recursive user.
OK. Let's focus on only each_recursive in this PR.
Co-authored-by: Sutou Kouhei <[email protected]>
|
I've merged. |
Fix #134
Summary
This PR does:
benchmark/each_recursive.yamlNode#each_recursiveimplementation for performanceNode#each_recursiveThe performance of
Node#each_recursiveis improved 60~80x faster.Details
each_recursiveis too much slow as I described in #134.I improved this performance by rewriting its implementation in this PR.
Also, I added a benchmark in
benchmark/each_recursive.yamland the following is a result on my laptop:We can see that the performance is improved 60~80x faster.
Additionally, I added a new test for
Node#each_recursive.It was missing, but we need it to confirm not to break the previous behavior.
Thank you.