From dab80658b684a093f4ef8b2c0b154df58aa710c9 Mon Sep 17 00:00:00 2001 From: Hiroya Fujinami Date: Fri, 7 Jun 2024 11:23:14 +0900 Subject: [PATCH] Improve `Node#each_recursive` performance (#139) Fix #134 ## 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. --------- Co-authored-by: Sutou Kouhei --- benchmark/each_recursive.yaml | 40 +++++++++++++++++++++++++++++++++++ lib/rexml/node.rb | 12 +++++++---- test/test_document.rb | 36 +++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 4 deletions(-) create mode 100644 benchmark/each_recursive.yaml diff --git a/benchmark/each_recursive.yaml b/benchmark/each_recursive.yaml new file mode 100644 index 00000000..c745f8ce --- /dev/null +++ b/benchmark/each_recursive.yaml @@ -0,0 +1,40 @@ +loop_count: 100 +contexts: + - gems: + rexml: 3.2.6 + require: false + prelude: require 'rexml' + - name: master + prelude: | + $LOAD_PATH.unshift(File.expand_path("lib")) + require 'rexml' + - name: 3.2.6(YJIT) + gems: + rexml: 3.2.6 + require: false + prelude: | + require 'rexml' + RubyVM::YJIT.enable + - name: master(YJIT) + prelude: | + $LOAD_PATH.unshift(File.expand_path("lib")) + require 'rexml' + RubyVM::YJIT.enable + +prelude: | + require 'rexml/document' + + xml_source = +"" + 100.times do + x_node_source = "" + 100.times do + x_node_source = "#{x_node_source}" + end + xml_source << x_node_source + end + xml_source << "" + + document = REXML::Document.new(xml_source) + +benchmark: + each_recursive: document.each_recursive { |_| } diff --git a/lib/rexml/node.rb b/lib/rexml/node.rb index 081caba6..c771db70 100644 --- a/lib/rexml/node.rb +++ b/lib/rexml/node.rb @@ -52,10 +52,14 @@ def parent? # Visit all subnodes of +self+ recursively def each_recursive(&block) # :yields: node - 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 end # Find (and return) first subnode (recursively) for which the block diff --git a/test/test_document.rb b/test/test_document.rb index 4bf3f55d..7fccbacb 100644 --- a/test/test_document.rb +++ b/test/test_document.rb @@ -209,6 +209,42 @@ def test_gt_linear_performance_attribute_value end end + def test_each_recursive + xml_source = <<~XML + + + + + + + + + + + + + + + + XML + + expected_names = %w[ + root + 1_1 1_2 1_3 + 2_1 2_2 2_3 + ] + + document = REXML::Document.new(xml_source) + + # Node#each_recursive iterates elements only. + # This does not iterate XML declerations, comments, attributes, CDATA sections, etc. + actual_names = [] + document.each_recursive do |element| + actual_names << element.attributes["name"] + end + assert_equal(expected_names, actual_names) + end + class WriteTest < Test::Unit::TestCase def setup @document = REXML::Document.new(<<-EOX)