Index each child list once in REXML::XPathParser.sort

benchmark/xpath_sort.yaml

@@ -0,0 +1,58 @@
+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'
+
+  # The sizes below are picked to keep one run short.  Both cases went from
+  # quadratic to linear, so the gap widens with the size: raising WIDTH or
+  # ITEMS makes "before" slower without making "after" much slower.
+
+  # Wide parent: REXML::XPathParser.sort keys every hit on its index under
+  # the parent, so the whole child list is walked once per hit.
+  #
+  # The hits must be scattered through the list, not gathered at its front:
+  # a hit near the front is found early and hides the cost of the walk.
+  WIDTH = 1000
+  xml_wide = '<root>' +
+             WIDTH.times.collect {|i| i % 7 == 0 ? '<leaf/>' : '<other/>' }.join +
+             '</root>'
+  doc_wide = REXML::Document.new(xml_wide)
+
+  # Predicate: string() takes the first node of its argument in document
+  # order, so it sorts once per candidate node, and every one of those sorts
+  # walks up to the wide parent the candidates hang off.
+  #
+  # Each item needs two <a>: with one, string()'s argument is a single node
+  # and sort returns it untouched, so no sorting happens at all.
+  ITEMS = 300
+  xml_items = '<root>' +
+              ITEMS.times.collect {|i| "<item><a>v#{i}</a><a>z</a></item>" }.join +
+              '</root>'
+  doc_items = REXML::Document.new(xml_items)
+
+benchmark:
+  "//leaf (hits scattered among a wide parent)": |
+    REXML::XPath.match(doc_wide, "//leaf")
+  "//item[string(a) = 'v3'] (sort per candidate)": |
+    REXML::XPath.match(doc_items, "//item[string(a) = 'v3']")

lib/rexml/functions.rb

@@ -14,6 +14,7 @@ def initialize
       @context = nil
       @namespace_context = {}
       @variables = {}
+      @node_indexes = nil
     end
 
     INTERNAL_METHODS = [
@@ -22,6 +23,7 @@ def initialize
       :variables,
       :variables=,
       :context=,
+      :node_indexes=,
       :target_named_node,
       :send,
       :compare_language,
@@ -42,6 +44,12 @@ def variables ; @variables ; end
 
     def context=(value); @context = value; end
 
+    # The evaluation's shared index cache, set by XPathParser so that sorting
+    # here does not re-index the same children for every call.  It is
+    # nil for the shared Functions instance, which is reachable without going
+    # through XPathParser; sort then caches for one call only.
+    def node_indexes=(value); @node_indexes = value; end
+
     # Returns the last node of the given list of nodes.
     def last( )
       @context[:size]
@@ -81,7 +89,7 @@ def target_named_node(node_set = nil)
         when nil
           @context[:node]
         when Array
-          XPathParser.sort(node_set).first
+          XPathParser.sort(node_set, @node_indexes).first
         end
       node if node.respond_to?(:namespace)
     end
@@ -138,7 +146,7 @@ def string( object=@context[:node] )
       else
         case object
         when Array
-          string(XPathParser.sort(object).first)
+          string(XPathParser.sort(object, @node_indexes).first)
         when Float
           if object.nan?
             "NaN"

lib/rexml/xpath_parser.rb

@@ -66,6 +66,7 @@ def initialize(strict: false)
       @attlist_mappings = nil
       @document = nil
       @element_namespaces_cache = {}
+      @node_indexes = nil
       @nest = 0
       @strict = strict
     end
@@ -151,14 +152,25 @@ def first( path_stack, node )
 
     def match(path_stack, node)
       @document = node.document
+      # A fresh cache per evaluation, so that a document modified between two
+      # evaluations is not ordered by stale indexes.
+      @node_indexes = {}.compare_by_identity
+      @functions.node_indexes = @node_indexes
       nodeset = [node]
       result = expr(path_stack, nodeset)
       case result
       when Array # nodeset
-        XPathParser.sort(result)
+        XPathParser.sort(result, @node_indexes)
       else
         [result]
       end
+    ensure
+      # Let go of the indexes: they are no use to the next evaluation, and the
+      # cache holds one entry per node it had to look up.  The document itself
+      # stays reachable through @document and @element_namespaces_cache, which
+      # outlive the evaluation.
+      @node_indexes = nil
+      @functions.node_indexes = nil
     end
 
     private
@@ -364,7 +376,7 @@ def apply_remaining_predicates(path_stack, value)
       value = [] unless value.is_a?(Array)
       path_stack.unshift(:node)
       step(path_stack) do
-        [:iterate_nodesets, [XPathParser.sort(value)]]
+        [:iterate_nodesets, [XPathParser.sort(value, @node_indexes)]]
       end
     end
 
@@ -809,38 +821,41 @@ def leave(tag, *args)
     ATTRIBUTE_POSITION = -1
     private_constant :ATTRIBUTE_POSITION
 
-    # Reorders an array of nodes so that they are in document order
-    # It tries to do this efficiently.
+    # Reorders an array of nodes so that they are in document order.
+    #
+    # Node sets are built up as unordered sets by the axis scanners, so they
+    # have to be put back into order here.  A node is keyed on the index it
+    # holds under each of its ancestors, outermost first, so that comparing
+    # two keys compares them at their first differing ancestor.
     #
-    # FIXME: I need to get rid of this, but the issue is that most of the XPath
-    # interpreter functions as a filter, which means that we lose context going
-    # in and out of function calls.  If I knew what the index of the nodes was,
-    # I wouldn't have to do this.  Maybe add a document IDX for each node?
-    # Problems with mutable documents.  Or, rewrite everything.
-    def self.sort(array_of_nodes)
+    # +node_indexes+ caches the index of every child and attribute that had to
+    # be looked up.  Pass the same one to every sort of an evaluation: a sort
+    # can happen many times per evaluation (Functions#string sorts once per
+    # candidate node), and those sorts usually revisit the same parents.
+    # Passing nothing just means the caching lasts for this one call.
+    def self.sort(array_of_nodes, node_indexes = nil)
       return array_of_nodes if array_of_nodes.size <= 1
-
-      attribute_positions = {}.compare_by_identity
+      node_indexes ||= {}.compare_by_identity
       array_of_nodes.sort_by do |node|
         if node.node_type == :attribute
           # An attribute has no place of its own in the child tree, so its key
           # extends that of the element carrying it.
-          ancestor_indexes(node.element) <<
-            ATTRIBUTE_POSITION << attribute_position(node, attribute_positions)
+          ancestor_indexes(node.element, node_indexes) <<
+            ATTRIBUTE_POSITION << attribute_position(node, node_indexes)
         else
-          ancestor_indexes(node)
+          ancestor_indexes(node, node_indexes)
         end
       end
     end
 
     # The index the node holds under each of its ancestors, outermost first.
-    def self.ancestor_indexes(node)
+    def self.ancestor_indexes(node, node_indexes)
       indexes = []
       # Walk all the way up to the document.  Stopping at the root element
       # would leave every node outside it, and the root itself, with the same
       # empty key, and ties are then broken arbitrarily.
       while (parent = node.parent)
-        indexes << parent.index(node)
+        indexes << child_index_of(node, parent, node_indexes)
         node = parent
       end
       indexes.reverse!
@@ -851,22 +866,39 @@ def self.ancestor_indexes(node)
     # leaves the relative order of those implementation dependent, but document
     # order is a total ordering, so they do need one; this keeps them in the
     # order they were written in.
-    def self.attribute_position(attribute, positions)
-      position = positions[attribute]
+    def self.attribute_position(attribute, node_indexes)
+      position = node_indexes[attribute]
       return position if position
 
-      # Index the whole attribute list at once.  A node set often holds every
-      # attribute of an element, and looking each one up on its own would make
-      # sorting quadratic in the number of attributes.
+      # Index the whole attribute list at once, for the same reason the child
+      # list is indexed at once: a node set often holds every attribute of an
+      # element, and looking each one up on its own is quadratic.
       i = 0
       attribute.element.attributes.each_attribute do |other|
-        positions[other] ||= i
+        node_indexes[other] ||= i
         i += 1
       end
-      positions[attribute]
+      node_indexes[attribute]
     end
     private_class_method :attribute_position
 
+    # Index the whole child list at once.  A node set normally has many nodes
+    # under the same few parents, and walking that list once per node is what
+    # made sorting quadratic in the number of children.
+    def self.child_index_of(node, parent, node_indexes)
+      index = node_indexes[node]
+      return index if index
+
+      # Iterate the parent rather than its #children, which hands out a copy.
+      # Keep the first index of a node, as Parent#index does: a child list is
+      # not supposed to hold the same node twice, but Parent lets it happen,
+      # and the rest of REXML reads such a node as being at the first of its
+      # positions.
+      parent.each_with_index {|child, i| node_indexes[child] ||= i }
+      node_indexes[node]
+    end
+    private_class_method :child_index_of
+
     # Scanner for descendant-or-self axis
     def descendant_or_self(nodeset, tester, selector)
       descendant(nodeset, tester, selector, include_self: true)

test/xpath/test_base.rb

@@ -1625,6 +1625,37 @@ def test_linear_performance_sort_attributes_of_one_element
       end
     end
 
+    def test_linear_performance_sort_children_of_one_parent
+      # Ordering the children of an element must not cost more than the
+      # children themselves: one whole list is indexed per parent, not one
+      # list per node.
+      omit("This is fragile on JRuby") if RUBY_ENGINE == "jruby"
+      seq = [1000, 5000, 10000, 20000, 40000]
+      build = ->(n) {
+        Document.new("<root>#{n.times.collect { "<x/>" }.join}</root>")
+      }
+      assert_linear_performance(seq, rehearsal: 10, pre: build) do |doc|
+        XPath.match(doc, "//x")
+      end
+    end
+
+    def test_linear_performance_sort_shared_within_one_evaluation
+      # A predicate sorts once per candidate node -- string() takes the first
+      # node of its argument in document order -- and every one of those sorts
+      # walks up to the wide parent the candidates hang off.  They must share
+      # the evaluation's index cache, or that parent is indexed once per
+      # candidate.
+      omit("This is fragile on JRuby") if RUBY_ENGINE == "jruby"
+      seq = [250, 1250, 2500, 5000, 10000]
+      build = ->(n) {
+        items = n.times.collect {|i| "<item><a>v#{i}</a><a>z</a></item>" }.join
+        Document.new("<root>#{items}</root>")
+      }
+      assert_linear_performance(seq, rehearsal: 10, pre: build) do |doc|
+        XPath.match(doc, "//item[string(a) = 'v3']")
+      end
+    end
+
     def test_document_order_attribute_axis_across_elements
       # Attributes of different elements are ordered by their owning elements.
       doc = Document.new("<root><a id='1'/><a id='2'/><a id='3'/></root>")
@@ -1644,6 +1675,32 @@ def test_document_order_mixed_text_and_element_children
                    stringify_nodes(nodes))
     end
 
+    def test_document_order_reflects_modification_between_matches
+      # Sorting caches the index of every child it looks up, and the cache
+      # lasts for one evaluation.  Reuse one parser, so that a cache held past
+      # an evaluation would be seen by the next one; XPath.match builds a new
+      # parser per call and could never show a stale cache.
+      doc = Document.new("<root><x id='1'/><x id='2'/><x id='3'/></root>")
+      parser = XPathParser.new
+      assert_equal(["1", "2", "3"], parser.parse("//x/@id", doc).collect(&:value))
+
+      # Move a node rather than add or remove one: inserting or deleting shifts
+      # every following sibling alike, which leaves their order intact even
+      # when the indexes are stale.
+      moved = doc.root.children[0]
+      doc.root.delete(moved)
+      doc.root.add(moved)
+      assert_equal(["2", "3", "1"], parser.parse("//x/@id", doc).collect(&:value))
+    end
+
+    def test_document_order_in_function_argument_node_set
+      # string() takes the first node of its argument in document order, and is
+      # evaluated once per candidate node.
+      doc = Document.new("<root><item><a>first</a><a>second</a></item></root>")
+      assert_equal(["item"], XPath.match(doc, "//item[string(a) = 'first']").collect(&:name))
+      assert_equal([], XPath.match(doc, "//item[string(a) = 'second']").collect(&:name))
+    end
+
     def test_unimplemented_id_should_not_contaminate_nil
       doc = Document.new("<root/>")
       assert_equal([], XPath.match(doc, 'id("foo")'))