diff --git a/benchmark/xpath_sort.yaml b/benchmark/xpath_sort.yaml
new file mode 100644
index 00000000..182380b6
--- /dev/null
+++ b/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 = '' +
+ WIDTH.times.collect {|i| i % 7 == 0 ? '' : '' }.join +
+ ''
+ 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 : with one, string()'s argument is a single node
+ # and sort returns it untouched, so no sorting happens at all.
+ ITEMS = 300
+ xml_items = '' +
+ ITEMS.times.collect {|i| "- v#{i}z
" }.join +
+ ''
+ 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']")
diff --git a/lib/rexml/functions.rb b/lib/rexml/functions.rb
index 106a21ee..f4bb4ad2 100644
--- a/lib/rexml/functions.rb
+++ b/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"
diff --git a/lib/rexml/xpath_parser.rb b/lib/rexml/xpath_parser.rb
index de8d41e3..bb655f51 100644
--- a/lib/rexml/xpath_parser.rb
+++ b/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)
diff --git a/test/xpath/test_base.rb b/test/xpath/test_base.rb
index a6e9655b..f0f53219 100644
--- a/test/xpath/test_base.rb
+++ b/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("#{n.times.collect { "" }.join}")
+ }
+ 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| "- v#{i}z
" }.join
+ Document.new("#{items}")
+ }
+ 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("")
@@ -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("")
+ 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("- firstsecond
")
+ 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("")
assert_equal([], XPath.match(doc, 'id("foo")'))