Improve `Node#each_recursive` performance #139

makenowjust · 2024-06-05T07:59:00Z

Summary

This PR does:

Add benchmark/each_recursive.yaml
Rewrite Node#each_recursive implementation for performance
Add a test for Node#each_recursive

The performance of Node#each_recursive is improved 60~80x faster.

Details

each_recursive is 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.yaml and the following is a result on my laptop:

RUBYLIB= BUNDLER_ORIG_RUBYLIB= /Users/makenowjust/Projects/github.com/makenowjust/simple-dotfiles/.asdf/installs/ruby/3.3.2/bin/ruby -v -S benchmark-driver /Users/makenowjust/Projects/github.com/ruby/rexml/benchmark/each_recursive.yaml
ruby 3.3.2 (2024-05-30 revision e5a195edf6) [arm64-darwin23]
Calculating -------------------------------------
                     rexml 3.2.6      master  3.2.6(YJIT)  master(YJIT)
      each_recursive      11.279     686.502       17.926        1.470k i/s -     100.000 times in 8.866303s 0.145666s 5.578360s 0.068018s

Comparison:
                   each_recursive
        master(YJIT):      1470.2 i/s
              master:       686.5 i/s - 2.14x  slower
         3.2.6(YJIT):        17.9 i/s - 82.01x  slower
         rexml 3.2.6:        11.3 i/s - 130.35x  slower

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.

Add `benchmark/each_recursive.yaml` Rewrite `Node#each_recursive` implementation for performance Add a test for `Node#each_recursive`

kou · 2024-06-06T00:50:41Z

lib/rexml/node.rb

-      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


Can we improve REXML::Elements#each(nil) case instead?

This seems a possible improvement. But, IMHO, it can be split into another pull request.

Do we still need to change REXML::Node#each_recursive when we improve REXML::Elements#each(nil)?

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).

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.

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.

Thank you for your understanding!

kou · 2024-06-06T00:54:48Z

test/test_document.rb

+          100.times do
+            x_node_source = ''
+            100.times do


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.

kou · 2024-06-06T00:56:18Z

test/test_document.rb

+          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)


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)

kou · 2024-06-06T07:11:20Z

lib/rexml/node.rb

-      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


Do we still need to change REXML::Node#each_recursive when we improve REXML::Elements#each(nil)?

test/test_document.rb

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

kou · 2024-06-07T01:55:45Z

lib/rexml/node.rb

-      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


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.

test/test_document.rb

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

kou · 2024-06-07T02:23:21Z

I've merged.
Thanks.

Improve Node#each_recursive performance

41e13e9

Add `benchmark/each_recursive.yaml` Rewrite `Node#each_recursive` implementation for performance Add a test for `Node#each_recursive`

kou reviewed Jun 6, 2024

View reviewed changes

Update each_recursive test

5d93a49

Use the small iteration numbers for test. Check names instead of counting elements. ruby#139 (comment) ruby#139 (comment)

kou reviewed Jun 6, 2024

View reviewed changes

makenowjust and others added 2 commits June 6, 2024 16:58

Update test/test_document.rb

3640dcc

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

Fix some code stylings

81f7227

kou reviewed Jun 7, 2024

View reviewed changes

Move test_each_recursive

f6efe5f

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

kou merged commit dab8065 into ruby:master Jun 7, 2024
55 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `Node#each_recursive` performance #139

Improve `Node#each_recursive` performance #139

makenowjust commented Jun 5, 2024 •

edited

Loading

kou Jun 6, 2024

makenowjust Jun 6, 2024

kou Jun 6, 2024

makenowjust Jun 6, 2024

makenowjust Jun 6, 2024

kou Jun 7, 2024

makenowjust Jun 7, 2024

kou Jun 6, 2024

kou Jun 6, 2024

kou Jun 6, 2024

kou Jun 7, 2024

kou commented Jun 7, 2024

Improve Node#each_recursive performance #139

Improve Node#each_recursive performance #139

Conversation

makenowjust commented Jun 5, 2024 • edited Loading

Summary

Details

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kou commented Jun 7, 2024

Improve `Node#each_recursive` performance #139

Improve `Node#each_recursive` performance #139

makenowjust commented Jun 5, 2024 •

edited

Loading