From f7ab510c2e3e2c327b94744b4b3f53d75219fced Mon Sep 17 00:00:00 2001 From: NAITOH Jun Date: Fri, 14 Aug 2026 22:38:14 +0900 Subject: [PATCH] Index each child list once in REXML::XPathParser.sort REXML::XPathParser.sort keys each node on the index it holds under each of its ancestors, and it asked Parent#index for every one of those, which walks the child list looking for the node. A node set normally has many nodes under the same few parents, so the same list was walked again and again and sorting was quadratic in the number of children of a wide parent. Walk a child list once and remember the index of every child in it, the way the attribute list already is. Sorting is then proportional to the children of the parents that the node set actually hangs off, rather than to those of any one of them squared. Children and attributes now share one cache, and it is passed to sort as an argument so that it can also be shared across the sorts of one evaluation. A sort can happen many times per evaluation -- Functions#string sorts once per candidate node -- and those sorts usually revisit the same parents. XPathParser makes a new cache for each evaluation, so a document modified between two evaluations is not ordered by stale indexes. Ordering is unchanged, including for a child list that holds the same node twice: the first index of a node is the one kept, which is the one Parent#index reports and the one Node#next_sibling_node and friends already assume. ## Benchmark ``` $ benchmark-driver benchmark/xpath_sort_latest.yaml Calculating ------------------------------------- before after before(YJIT) after(YJIT) //leaf (hits scattered among a wide parent) 373.713 952.844 534.805 1.257k i/s - 100.000 times in 0.267585s 0.104949s 0.186984s 0.079572s //item[string(a) = 'v3'] (sort per candidate) 125.080 176.305 194.029 266.828 i/s - 100.000 times in 0.799488s 0.567199s 0.515387s 0.374773s Comparison: //leaf (hits scattered among a wide parent) after(YJIT): 1256.7 i/s after: 952.8 i/s - 1.32x slower before(YJIT): 534.8 i/s - 2.35x slower before: 373.7 i/s - 3.36x slower //item[string(a) = 'v3'] (sort per candidate) after(YJIT): 266.8 i/s before(YJIT): 194.0 i/s - 1.38x slower after: 176.3 i/s - 1.51x slower before: 125.1 i/s - 2.13x slower ``` - YJIT=ON : 1.38x - 2.35x faster - YJIT=OFF : 1.41x - 2.55x faster Speed is bought with memory: //leaf over a document of 20000 children holds 20001 indexes, about 0.9MB. It cannot grow past one entry per node of the document, whatever the path asks for, and match drops it on the way out rather than carry it into the next evaluation. That does not free the document: a parser holds the one it last queried through @document and @element_namespaces_cache, neither of which the evaluation clears. The FIXME asking for a document index per node goes away with it. It worried about mutable documents, which is why the cache lives for one evaluation only. Within one evaluation the tree is taken to be fixed: one changed part way through may still be ordered by indexes taken before the change. --- benchmark/xpath_sort.yaml | 58 ++++++++++++++++++++++++++++ lib/rexml/functions.rb | 12 +++++- lib/rexml/xpath_parser.rb | 80 +++++++++++++++++++++++++++------------ test/xpath/test_base.rb | 57 ++++++++++++++++++++++++++++ 4 files changed, 181 insertions(+), 26 deletions(-) create mode 100644 benchmark/xpath_sort.yaml 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")'))