Skip to content

Optimize REXML::XPathParser#sort with depth-first search - #314

Draft
naitoh wants to merge 1 commit into
ruby:masterfrom
naitoh:fix_XPathParser_sort_performance_xpath
Draft

Optimize REXML::XPathParser#sort with depth-first search#314
naitoh wants to merge 1 commit into
ruby:masterfrom
naitoh:fix_XPathParser_sort_performance_xpath

Conversation

@naitoh

Copy link
Copy Markdown
Contributor

Replace the O(n x depth) algorithm that walked the parent chain for every node calling parent.index(self) at each level, with a single O(N) depth-first search (DFS) that assigns an integer document-order position to every node in the tree, then sorts by those integers with sort_by.

New helpers:

  • sort_anchor: maps attribute nodes to their owner element so attributes sort by their element document position (matching the previous behavior).
  • document_order_positions: iterative DFS from each unique root using an explicit stack, assigning monotonically increasing counters in document order. Multiple disjoint subtrees are handled by tracking visited roots.

Benchmark

$ benchmark-driver benchmark/xpath.yaml
before after before(YJIT) after(YJIT) REXML::XPath.match(REXML::Document.new(xml), 'a//a') 2.872k 3.687k 3.514k 4.110k i/s - 100.000 times in 0.034820s 0.027124s 0.028461s 0.024330s
REXML::XPath.match(REXML::Document.new(xml), '//a//a') 898.013 1.148k 1.194k 1.388k i/s - 100.000 times in 0.111357s 0.087108s 0.083734s 0.072023s
REXML::Document.new(xml_wide).root.children.first.next_sibling_node 7.143M 7.692M 277.778k 281.690k i/s - 100.000 times in 0.000014s 0.000013s 0.000360s 0.000355s
REXML::Document.new(xml_wide).root.children.last.previous_sibling_node 42.662k 43.029k 52.743k 51.151k i/s - 100.000 times in 0.002344s 0.002324s 0.001896s 0.001955s
Comparison:
REXML::XPath.match(REXML::Document.new(xml), 'a//a')
after(YJIT): 4110.2 i/s after: 3686.8 i/s - 1.11x slower
before(YJIT): 3513.6 i/s - 1.17x slower
before: 2871.9 i/s - 1.43x slower
REXML::XPath.match(REXML::Document.new(xml), '//a//a')
after(YJIT): 1388.4 i/s before(YJIT): 1194.3 i/s - 1.16x slower
after: 1148.0 i/s - 1.21x slower
before: 898.0 i/s - 1.55x slower
REXML::Document.new(xml_wide).root.children.first.next_sibling_node
after: 7692111.4 i/s before: 7143040.3 i/s - 1.08x slower
after(YJIT): 281690.4 i/s - 27.31x slower
before(YJIT): 277777.8 i/s - 27.69x slower
REXML::Document.new(xml_wide).root.children.last.previous_sibling_node
before(YJIT): 52742.6 i/s after(YJIT): 51150.9 i/s - 1.03x slower
after: 43029.3 i/s - 1.23x slower
before: 42662.1 i/s - 1.24x slower
  • YJIT=ON : 1.00x - 1.17x faster
  • YJIT=OFF : 1.01x - 1.28x faster

Replace the O(n x depth) algorithm that walked the parent chain for every
node calling parent.index(self) at each level, with a single O(N)
depth-first search (DFS) that assigns an integer document-order position
to every node in the tree, then sorts by those integers with sort_by.
New helpers:
- sort_anchor: maps attribute nodes to their owner element so attributes
sort by their element document position (matching the previous behavior).
- document_order_positions: iterative DFS from each unique root using an
explicit stack, assigning monotonically increasing counters in document
order. Multiple disjoint subtrees are handled by tracking visited roots.
@naitoh
naitoh requested a review from kouMay 20, 2026 23:58
@kou
kou requested a review from CopilotMay 21, 2026 01:45

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR optimizes REXML::XPathParser#sort by replacing repeated parent-chain/index lookups with a single DFS pass that assigns document-order positions and then sorts nodes by those positions, improving XPath evaluation performance for large nodesets.

Changes:

  • Reimplemented XPathParser#sort using a DFS-based document-order position map (document_order_positions).
  • Added sort_anchor to preserve prior attribute ordering semantics by sorting attributes by their owning element’s position.
  • Added regression tests to ensure correct document ordering for attribute axes across elements and mixed text/element child nodes.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

FileDescription
lib/rexml/xpath_parser.rbReplaces sort implementation with DFS-based document-order position assignment and sorting.
test/xpath/test_base.rbAdds tests covering attribute-axis ordering across elements and mixed child-node ordering.
benchmark/xpath.yamlIncreases benchmark depth/width parameters to better stress ordering and traversal performance.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +678 to +685
nodes.each do |node|
anchor = sort_anchor(node)
root = anchor
while (parent = root.parent)
root = parent
end
new_arry << [ node_idx.reverse, node ]
}
ordered = new_arry.sort_by do |index, node|
if order == :forward
index
else
index.map(&:-@)
next if visited_roots.key?(root)
visited_roots[root] = true
@tompng

Copy link
Copy Markdown
Member

How about cache the result of document_sort_position and reuse it?
There should be only one root node (which is the root parent of a node passed to match(xpath, node))

defdocument_order_positions(nodes); ...;enddefdocument_order_positions()# no argument neededreturncached_resultifalready_calculatedroot=node_passed_to_matchroot=root.parentwhileroot.parent# traverse from rootend

There is one more strategy:

  1. Traverse up from sort target nodes until reaching a common ancestor
  2. Create a small tree that only contain nodes traversed in step 1
  3. Traverse the tree with DFS, calculate position for each node in this small tree

This tree can't be reused, but constructing and assigning position number may be fast when nodes.size is small.
If we're not reusing the result, this approach might be better.

@naitoh
naitoh marked this pull request as draft June 8, 2026 12:39
@tompng

Copy link
Copy Markdown
Member

Since #330, sort now runs only once per XPath evaluation (on the final result) in the usual case, so my earlier caching suggestion no longer applies. Please ignore it 🙇.
The simple DFS approach here looks good to me.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@naitoh@tompng