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")'))