Skip to content

Optimize XPath step - #315

Merged
naitoh merged 3 commits into
ruby:masterfrom
tompng:xpath_step_optimize
Jun 14, 2026
Merged

Optimize XPath step#315
naitoh merged 3 commits into
ruby:masterfrom
tompng:xpath_step_optimize

Conversation

@tompng

@tompngtompng commented May 21, 2026

Copy link
Copy Markdown
Member

Refactor step so that nodeset materialization is deferred. Instead of building the full nodeset up front and filtering through predicates,
each axis returns a scan descriptor ([generator_name, generator_argument]), and step picks a scan strategy based on the predicates' shape.

Predicates are classified into three groups:

kindexamplesstrategy passed to the generator
position-independent[@a="1"], [name()="foo"], [@a=@b]:uniq — emit deduplicated matching nodes
simple positional[N], [position()=N], [position()>N], [position()<N][op, value] — positional scan with one comparison
complex / position-dependent[position()*@a], [last()-1], ...:nodesets — fall back to per-anchor nodesets + the previous evaluate_predicate pipeline

Mixed predicate lists are split: position-independent predicates before the first positional predicate are folded into the node test;
predicates after it are applied per-node on the result.

Each axis can implement zero, one, or all of the three strategies. If a strategy is not implemented, the generator falls back to producing
:nodesets and the common slow path (non_optimized_nodesets_select) handles dedup / positional filtering on flattened nodesets — i.e. the
same behavior as before this PR.

This pull request adds fast paths for:

  • descendant / descendant-or-self: :uniq (single DFS with a seen-set; this is what speeds up //a//a//a//a)
  • ancestor / ancestor-or-self: :uniq (parent-chain walk with a seen-set)
  • preceding-sibling / following-sibling: :uniq and [op, value] (sibling scan with anchor-index tracking)

Other axes (child, parent, self, attribute, preceding, following, etc) keeps the previous behavior via the fallback path; they can be optimized in follow-ups without changing call sites.

Detail

For //a//a//a style queries, the previous code built nodesets keyed by each anchor, including the same descendant once per anchor. The new
:uniq path scans every node at most once per step.

For [position() > N] style predicates on wide trees (e.g. //a/preceding-sibling::*[position()>2]), we previously built the full
preceding-sibling nodeset for each anchor and then ran evaluate_predicate. The new [op, value] path scans children once per parent and uses
anchor-index bookkeeping to recover per-anchor positions.

Note: general XPath cannot be linear — e.g. *[position() * number(@a) % number(@b) = 1] is genuinely O(n²) — so the goal is only to add a fast-path for specific case: position-independent predicates and simple-positional predicates.

Benchmark of best case

DEPTH=500xml='<a>' * DEPTH + '</a>' * DEPTHdoc=REXML::Document.new(xml)WIDTH=1000xml_wide='<root>' + '<child/>' * WIDTH + '</root>'doc_wide=REXML::Document.new(xml_wide)REXML::XPath.match(doc,"//a//a");# processing time: 30.756939s → 0.126807sREXML::XPath.match(doc_wide,"//*/preceding-sibling::*[position()=10]");# processing time: 2.446333s → 0.083954s

Benchmark of various case

Scenario

prelude: | require "rexml" xml_wide = "<root>" + (1..1000).map { |i| "<item id='#{i}'/>" }.join + "</root>" wide = REXML::Document.new(xml_wide) xml_deep = "<root>" + (1..1000).map { |i| "<item id='#{i}'>" }.join + '</item>'*1000 + "</root>" deep = REXML::Document.new(xml_deep)benchmark:
child: REXML::XPath.match(wide, "root/item")descendant: REXML::XPath.match(deep, "//item")descendant-descendant: REXML::XPath.match(deep, "//item//item")descendant-descendant-wildcard: REXML::XPath.match(deep, "//*//*")ancestor-descendant: REXML::XPath.match(deep, "descendant::*/ancestor::*/descendant::*")preceding-following-sibling: REXML::XPath.match(wide, "//*/preceding-sibling::*/following-sibling::*")preceding-following-sibling-positional: REXML::XPath.match(wide, "//*/preceding-sibling::*[10]/following-sibling::*[10]")

Compares

master, xpath_step_optimize (this pull), sort_on_demand(#330), sort_improve(Emulate ideal sort computation time), and its combinations.

There's no implementation of sort_improve yet, so I used the code below to emulate the computational cost of ideal sort.

defsort(array_of_nodes)# Just spend time to emulate the ideal computational cost of sorting nodesparents=Set.new.compare_by_identityarray_of_nodes.each{parents << it.parentifit.parent}4.timesdo# find the common ancestornodes=array_of_nodesseen=Set.new.compare_by_identitywhilenodes.size >= 2new_nodes=Set.new.compare_by_identitynodes.map(&:parent).eachdo |parent|
ifparent && !seen.include?(parent)seen << parentnew_nodes << parentendendnodes=new_nodesend# iterate each node's siblingsparents.each{it.children.each{}}endarray_of_nodes# not sortedend

Result

Comparison:
child
master: 1288.1 i/s master_sort_improve: 1190.4 i/s - 1.08x slower
xpath_step_optimize_sort_on_demand_sort_improve: 875.3 i/s - 1.47x slower
xpath_step_optimize_sort_improve: 861.3 i/s - 1.50x slower
xpath_step_optimize_sort_on_demand: 92.3 i/s - 13.96x slower
xpath_step_optimize: 91.7 i/s - 14.05x slower
sort_on_demand: 90.6 i/s - 14.21x slower
descendant
master_sort_improve: 75.5 i/s xpath_step_optimize_sort_on_demand_sort_improve: 75.1 i/s - 1.01x slower
xpath_step_optimize_sort_improve: 68.8 i/s - 1.10x slower
sort_on_demand: 21.4 i/s - 3.52x slower
xpath_step_optimize_sort_on_demand: 21.4 i/s - 3.52x slower
master: 20.9 i/s - 3.61x slower
xpath_step_optimize: 11.7 i/s - 6.45x slower
descendant-descendant
xpath_step_optimize_sort_on_demand_sort_improve: 47.5 i/s xpath_step_optimize_sort_improve: 41.9 i/s - 1.13x slower
xpath_step_optimize_sort_on_demand: 17.9 i/s - 2.65x slower
master_sort_improve: 8.6 i/s - 5.54x slower
sort_on_demand: 6.7 i/s - 7.07x slower
xpath_step_optimize: 6.1 i/s - 7.84x slower
master: 4.6 i/s - 10.24x slower
descendant-descendant-wildcard
xpath_step_optimize_sort_on_demand_sort_improve: 339.5 i/s xpath_step_optimize_sort_improve: 155.9 i/s - 2.18x slower
xpath_step_optimize_sort_on_demand: 26.4 i/s - 12.86x slower
master_sort_improve: 10.0 i/s - 33.96x slower
sort_on_demand: 7.7 i/s - 44.30x slower
xpath_step_optimize: 6.8 i/s - 50.06x slower
master: 4.9 i/s - 68.58x slower
ancestor-descendant
xpath_step_optimize_sort_on_demand_sort_improve: 377.9 i/s xpath_step_optimize_sort_improve: 203.7 i/s - 1.85x slower
xpath_step_optimize_sort_on_demand: 26.3 i/s - 14.39x slower
xpath_step_optimize: 8.7 i/s - 43.24x slower
master_sort_improve: 7.8 i/s - 48.55x slower
sort_on_demand: 6.3 i/s - 59.93x slower
master: 5.0 i/s - 75.46x slower
preceding-following-sibling
xpath_step_optimize_sort_on_demand_sort_improve: 684.1 i/s xpath_step_optimize_sort_improve: 424.5 i/s - 1.61x slower
xpath_step_optimize_sort_on_demand: 85.8 i/s - 7.98x slower
xpath_step_optimize: 23.7 i/s - 28.91x slower
master_sort_improve: 20.9 i/s - 32.72x slower
sort_on_demand: 19.3 i/s - 35.39x slower
master: 13.9 i/s - 49.11x slower
preceding-following-sibling-positional
xpath_step_optimize_sort_on_demand_sort_improve: 425.4 i/s xpath_step_optimize_sort_improve: 315.0 i/s - 1.35x slower
xpath_step_optimize_sort_on_demand: 84.3 i/s - 5.05x slower
xpath_step_optimize: 23.3 i/s - 18.22x slower
master_sort_improve: 2.1 i/s - 201.38x slower
sort_on_demand: 2.1 i/s - 204.75x slower
master: 1.9 i/s - 222.08x slower

In scenario "child" and "descendant", this PR is slower than master because it adds one additional sort call. The difference will be small when sort is improved.
In most case, this PR itself does not unleash its full potential because sort is the next bottleneck. Combining with sort improvement is important.
The difference of "descendant-descendant" and "descendant-descendant-wildcard" shows that after optimizing sort, the bottleneck will be namespace lookup in qname check for deeply nested xml.

CopilotAI review requested due to automatic review settings May 21, 2026 13:11

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 refactors XPath step evaluation in REXML::XPathParser to defer nodeset materialization and introduce axis “scan strategies” that can fast-path common predicate shapes (position-independent and simple positional predicates), improving performance for deep and wide-tree queries.

Changes:

  • Reworked step to drive axis scans via [scanner_method, scanner_argument] and choose scanning strategies (:uniq, [op, value], :nodesets) based on predicate classification.
  • Added optimized scanners for descendant(-or-self), ancestor(-or-self), and sibling axes, plus a shared fallback selection path.
  • Added/updated XPath predicate and sibling-axis tests, including float literal predicates and positional comparisons.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

FileDescription
lib/rexml/xpath_parser.rbImplements deferred scanning/strategy selection in step, predicate classification, and optimized axis scanners.
test/xpath/test_predicate.rbAdds coverage for float-literal positional predicates and variable-as-position predicates.
test/xpath/test_base.rbAdds test for following-sibling::* with position() < N and adjusts nested-predicate test behavior.
test/xpath/test_axis_preceding_sibling.rbAdds tests for preceding-sibling:: with < and <= position predicates.
Comments suppressed due to low confidence (1)

lib/rexml/xpath_parser.rb:834

  • descendant axis fast path calls raw_node.children when include_self is false without checking the node type or whether children exists. If the context node is not an element/document (e.g., an attribute node), this will raise NoMethodError; previously it would just yield an empty descendant set. Guard this branch so it only iterates children for element/document nodes (or use the same node_type check as in recursive).
 raw_nodes.each do |raw_node|
if include_self
recursive.call(raw_node)
else
raw_node.children.each(&recursive)
end

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

Comment threadlib/rexml/xpath_parser.rb Outdated
Comment threadlib/rexml/xpath_parser.rb Outdated
Comment threadlib/rexml/xpath_parser.rb Outdated
@tompng
tompngforce-pushed the xpath_step_optimize branch from 6f94f2e to c4a1440CompareMay 21, 2026 15:57
CopilotAI review requested due to automatic review settings May 21, 2026 16:00
@tompng
tompngforce-pushed the xpath_step_optimize branch from c4a1440 to 67e3270CompareMay 21, 2026 16:00
@tompng
tompngforce-pushed the xpath_step_optimize branch from 67e3270 to 593620bCompareMay 21, 2026 16:06

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

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comment threadlib/rexml/xpath_parser.rb Outdated
Comment threadlib/rexml/xpath_parser.rb Outdated
@tompng
tompngforce-pushed the xpath_step_optimize branch from 593620b to 65c5103CompareMay 21, 2026 16:31
CopilotAI review requested due to automatic review settings May 21, 2026 17:11
@tompng
tompngforce-pushed the xpath_step_optimize branch from 65c5103 to 15a8f52CompareMay 21, 2026 17:11

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

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Comment threadlib/rexml/xpath_parser.rb Outdated
Comment threadlib/rexml/parsers/xpathparser.rb Outdated
Comment threadlib/rexml/xpath_parser.rb Outdated
@tompng
tompng marked this pull request as ready for review May 21, 2026 17:26
@tompng
tompngforce-pushed the xpath_step_optimize branch from 15a8f52 to 020085aCompareMay 21, 2026 18:13
CopilotAI review requested due to automatic review settings May 26, 2026 13:20
@tompng
tompngforce-pushed the xpath_step_optimize branch from 020085a to 926e521CompareMay 26, 2026 13:20

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

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

Comment threadlib/rexml/xpath_parser.rb
Comment threadlib/rexml/xpath_parser.rb Outdated
Comment threadlib/rexml/xpath_parser.rb Outdated
Comment threadtest/xpath/test_base.rb Outdated
Comment threadlib/rexml/xpath_parser.rb Outdated
Comment threadlib/rexml/xpath_parser.rb Outdated
Comment on lines 636 to 646
seen = {}.compare_by_identity
nodesets.each do |nodeset|
nodeset.each do |node|
raw_node = node.respond_to?(:raw_node) ? node.raw_node : node
seen[raw_node] = true
end
end
ordered = sort(seen.keys)
ordered.map.with_index(1) do |raw_node, position|
XPathNode.new(raw_node, position: position)
end

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

REXML previously skipped nodesets ordering when nodesets.size == 1, but it turned out that it was a bug. nodesets.first is sometimes sorted in reverse-document-order while the order should be document-order.

We can reduce sort in the future such as:

  • Add an ordered/reverse-ordered/unordered flag to nodeset
  • Sort only on the final result and before applying filter predicates

But this kind of separate optimization should be out of scope of this PR.

@tompng
tompngforce-pushed the xpath_step_optimize branch from 926e521 to 73dd670CompareMay 26, 2026 16:07
@tompng
tompngforce-pushed the xpath_step_optimize branch from 73dd670 to a6476a2CompareJune 3, 2026 17:33
CopilotAI review requested due to automatic review settings June 4, 2026 10:44
@tompng
tompngforce-pushed the xpath_step_optimize branch from a6476a2 to c2d8012CompareJune 4, 2026 10:44

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

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Comment threadlib/rexml/xpath_parser.rb Outdated
Comment threadlib/rexml/xpath_parser.rb
Comment on lines +446 to +452
when :nodesets
nodesets = nodeset.map do |node|
parent = node.parent
index = parent.children.index(node)
reverse ? parent.children[0...index].reverse : parent.children[index + 1..-1]
end
non_optimized_nodesets_select(nodesets, tester, selector)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

No. This is a non-optimizable slow path that needs O(n^2) calculation cost just like before.

Comment threadlib/rexml/xpath_parser.rb
Comment threadlib/rexml/xpath_parser.rb Outdated
@tompng
tompngforce-pushed the xpath_step_optimize branch from c2d8012 to 42c4307CompareJune 4, 2026 10:51
CopilotAI review requested due to automatic review settings June 4, 2026 10:58
@tompng
tompngforce-pushed the xpath_step_optimize branch from 42c4307 to 2f6ea03CompareJune 4, 2026 10:58

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

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Comment threadlib/rexml/xpath_parser.rb Outdated
Comment on lines +454 to +469
operator, value = selector
nodeset.group_by(&:parent).flat_map do |parent, sibling_nodes|
anchors = {}.compare_by_identity
sibling_nodes.each {|sibling| anchors[sibling] = true }
children = parent.children
children = children.reverse if reverse
followings = children.drop_while {|child| !anchors.key?(child) }.drop(1)
anchor_indexes = { 0 => true }
last_anchor = 0
index = 0
matched = []
followings.each do |node|
if tester.call(node)
case operator
when :==
matched << node if anchor_indexes.include?(index - value + 1)
Comment threadlib/rexml/xpath_parser.rb
Comment threadlib/rexml/xpath_parser.rb Outdated
@tompngtompng mentioned this pull request Jun 5, 2026
CopilotAI review requested due to automatic review settings June 9, 2026 16:17
@tompng
tompngforce-pushed the xpath_step_optimize branch from 739cba0 to d6afdc4CompareJune 9, 2026 16:17

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

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Comment on lines +226 to +235
parents = Set.new.compare_by_identity
nodeset.each do |node|
if node.node_type == :attribute
parent = node.element
else
parent = node.parent
end
nodesets << [parent] if parent
parents << parent
end
nodesets
[:iterate_nodesets, parents.map {|parent| [parent] }]
Comment threadlib/rexml/xpath_parser.rb
Comment on lines +601 to 605
new_nodeset = sort(nodes.to_a)
ensure
leave(:step, path_stack, new_nodeset) if @debug
end
end
Comment threadlib/rexml/xpath_parser.rb
Comment threadtest/xpath/test_attribute.rb Outdated
@tompngtompng mentioned this pull request Jun 9, 2026
@tompng
tompngforce-pushed the xpath_step_optimize branch from d6afdc4 to 80e6d0dCompareJune 9, 2026 17:28
CopilotAI review requested due to automatic review settings June 10, 2026 12:42

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

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comment threadlib/rexml/xpath_parser.rb
Comment threadtest/xpath/test_attribute.rb Outdated
If a predicate of xpath is position-independent, we don't need to create nodesets that has many duplicated nodes with different positions.
Implements optimization of preceding-sibling/following-sibling with simple positional predicates as an example.
We can add more optimizations for other axis in the future.
@tompng
tompngforce-pushed the xpath_step_optimize branch from a9873fd to b7d96b3CompareJune 10, 2026 13:10
@naitoh

Copy link
Copy Markdown
Contributor

@tompng

Remove XPathNode

@tompng

Copy link
Copy Markdown
MemberAuthor

Done removing that section

Comment threadlib/rexml/xpath_parser.rb Outdated
Comment threadlib/rexml/xpath_parser.rb Outdated
Comment threadlib/rexml/xpath_parser.rb Outdated
CopilotAI review requested due to automatic review settings June 13, 2026 07:15

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

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Comment threadlib/rexml/xpath_parser.rb Outdated
Comment threadlib/rexml/xpath_parser.rb
Comment threadlib/rexml/xpath_parser.rb
@tompng
tompngforce-pushed the xpath_step_optimize branch from f9a7d47 to ea768f4CompareJune 13, 2026 07:31
CopilotAI review requested due to automatic review settings June 13, 2026 07:32
@tompng
tompngforce-pushed the xpath_step_optimize branch from ea768f4 to a390822CompareJune 13, 2026 07:32
@tompng
tompngforce-pushed the xpath_step_optimize branch from a390822 to 447c0a3CompareJune 13, 2026 07:34

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

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Comment on lines +497 to +506
# Scanner for ancestor axis
def ancestor(nodeset, tester, selector, include_self: false)
nodeset = nodeset.select {|node| node.respond_to?(:parent) && node.parent }
case selector
when :uniq
ancestors = Set.new.compare_by_identity
nodeset.each do |node|
ancestors << node if include_self
parent = node.parent
while parent
Comment threadlib/rexml/xpath_parser.rb
Comment threadlib/rexml/xpath_parser.rb
Comment threadlib/rexml/xpath_parser.rb Outdated
Comment on lines +565 to +575
def split_positional_predicates(predicates)
pre_independent = predicates.take_while {|predicate| position_dependency(predicate).nil? }
predicates = predicates.drop(pre_independent.size)
return [pre_independent, nil, [], nil] if predicates.empty?

op = position_operation(predicates.first)
if op && predicates[1..-1].all? {|predicate| position_dependency(predicate).nil? }
[pre_independent, op, predicates[1..-1], nil]
else
[pre_independent, nil, nil, predicates]
end
Stop classifying dependency pattern, just return true(maybe dependent) of false(guaranteed to be independent)
CopilotAI review requested due to automatic review settings June 13, 2026 08:06

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

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Comment threadlib/rexml/xpath_parser.rb
Comment on lines +381 to +386
# Recursively checks if the expression contains position-dependent functions such as position() or last()
def calls_position_dependent_function?(expr)
return false unless Array === expr
return true if expr[0] == :function && (expr[1] == 'position' || expr[1] == 'last')
expr.any? {|part| calls_position_dependent_function?(part) }
end
Comment on lines +41 to +44
assert_equal(["5"],
XPath.match(context, "preceding-sibling::f[position() < 2]").map {|n| n.attributes["id"] })
assert_equal(["4", "5"],
XPath.match(context, "preceding-sibling::f[position() < 3]").map {|n| n.attributes["id"] })
array_of_nodes.each { |node|
node_idx = []
np = node.node_type == :attribute ? node.element : node
while np.parent and np.parent.node_type == :element

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The order of the nodes directly under the Document is incorrect.

Suggested change
whilenp.parentand(np.parent.node_type == :elementornp.parent.node_type == :document)
  • before
> xml='<!--c1--><!--c2--><!--c3--><root><x/><y/></root><!--c4--><!--c5-->'
> REXML::XPath.match(REXML::Document.new(xml),"/root/preceding-sibling::comment()").map(&:string)=>["c1","c2","c3"]
> REXML::XPath.match(REXML::Document.new(xml),"/root/preceding::node()").map(&:string)=>["c1","c2","c3"]
> REXML::XPath.match(REXML::Document.new(xml),"/root/following-sibling::comment()").map(&:string)=>["c4","c5"]
> REXML::XPath.match(REXML::Document.new(xml),"/descendant-or-self::node()")=>[<UNDEFINED> ... </>, <?xml ... ?>, #<REXML::Comment:0x0000000124a9b530 @parent=<UNDEFINED> ... </>,@string="c1">,#<REXML::Comment:0x0000000124a9b198 @parent=<UNDEFINED> ... </>, @string="c2">,#<REXML::Comment:0x0000000124a9af90 @parent=<UNDEFINED> ... </>, @string="c3">,
<root> ... </>, <x/>,
<y/>,#<REXML::Comment:0x0000000124a9a180 @parent=<UNDEFINED> ... </>, @string="c4">,#<REXML::Comment:0x0000000124a99fc8 @parent=<UNDEFINED> ... </>, @string="c5">]
  • after(this PR)
> xml='<!--c1--><!--c2--><!--c3--><root><x/><y/></root><!--c4--><!--c5-->'
> REXML::XPath.match(REXML::Document.new(xml),"/root/preceding-sibling::comment()").map(&:string)=>["c3","c2","c1"]
> REXML::XPath.match(REXML::Document.new(xml),"/root/preceding::node()").map(&:string)=>["c3","c2","c1"]
> REXML::XPath.match(REXML::Document.new(xml),"/root/following-sibling::comment()").map(&:string)=>["c4","c5"]
> REXML::XPath.match(REXML::Document.new(xml),"/descendant-or-self::node()")=>[#<REXML::Comment:0x0000000124546aa8 @parent=<UNDEFINED> ... </>, @string="c5">,#<REXML::Comment:0x0000000124547868 @parent=<UNDEFINED> ... </>, @string="c1">,#<REXML::Comment:0x0000000124547660 @parent=<UNDEFINED> ... </>, @string="c2">,#<REXML::Comment:0x0000000124547458 @parent=<UNDEFINED> ... </>, @string="c3">,
<root> ... </>, #<REXML::Comment:0x0000000124546c60 @parent=<UNDEFINED> ... </>,@string="c4">,
<UNDEFINED> ... </>, <x/>,
<y/>]

@tompngtompngJun 13, 2026

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It's an existing bug of sort

# master(7d9e7c2e08d81b688ab6a9d0d1b329b619d1ca55)xml='<!--c1--><!--c2--><!--c3--><foo><x/><y/><inner><!--comment--><foo/></inner></foo><!--c4--><!--c5-->'doc=REXML::Document.new(xml)REXML::XPath.match(doc,"/foo/preceding-sibling::comment()").map(&:string)# => ["c1", "c2", "c3"] # This was OK because sort is skipped when nodesets.size==1 in masterREXML::XPath.match(doc,"//foo/preceding-sibling::comment()").map(&:string)# => ["c3", "c2", "c1", "comment"] # If there's more than two `//foo` in the document, it'll run into the bug.

There's at least three bugs related to sort

  1. Unable to sort root comments because all sort keys are []
  2. Sort key of attribute is the same as attribute.element's key
  3. Sorting union of attributes and elements won't work because of 2, but before that, the sorting of union wasn't even performed
# master and in v3.4.4# Bug 1xml='<foo><foo/><!--c0--></foo>' + 100.times.map{"<!--c#{it+1}-->"}.joindoc=REXML::Document.new(xml)REXML::XPath.match(doc,"//foo/following-sibling::comment()").map(&:string)# => ["c77", "c2", "c3", "c4", ..., "c75", "c76", "c1", "c78", "c79", ..., "c99", "c100", "c0"]# Bug 2xml="<root>#{2.times.map{|i| "<foo #{50.times.map{"a#{50*i+it}='true'"}.join(' ')}/>"}.join}</root>"doc=REXML::Document.new(xml)REXML::XPath.match(doc,"//foo/attribute::*").map(&:name)# => ["a0", "a2", "a3", ..., "a48", "a49", "a1", "a99", "a50", "a51", "a52", ..., "a97", "a98"]# Bug 3xml="<root><a attr1='1' attr2='2'/><b/><c/></root>"doc=REXML::Document.new(xml)REXML::XPath.match(doc,"//c | //attribute::attr2 | //a | //attribute::attr1 | //b").map(&:name)# => ["c", "attr2", "a", "attr1", "b"] # not evne sorted# => ["attr2", "a", "attr1", "b", "c"] # Wrong even after changing to `sort(left | right)`

I think it can be fix it in a separate pull request, or perhaps leave it until rewriting sort with a performant algorithm.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK, I see.

@naitohnaitoh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I understand this PR scan strategy.
Thank you!

@naitoh
naitoh merged commit a6aa43c into ruby:masterJun 14, 2026
72 of 73 checks passed
@tompng
tompng deleted the xpath_step_optimize branch June 14, 2026 03:33
naitoh pushed a commit that referenced this pull request Jun 17, 2026
Delay sorting, only sort when it is needed.
In most case, sorting nodeset is not needed. Sort is only required in:
- Final result
- Creating nodesets(each nodeset should be axis-ordered) from a single
nodeset
- Ideally, this can be skipped if the following predicate is not
position-dependent
- Nodeset passed to a function (first node in document order is used)
### Number of sort operations
| XPath | master(before #315) | master(after #315) | this PR |
| --- | --- | --- | --- |
| `/a/b/c/d/e` | 3 | 4 | 1 |
| `(a/b/c/d)[position()>1]/e/f/g` | 5 | 7 | 2 |
| `number(/a/b/c/d/e)` | 3 | 4 |1 |
| `count(/a/b/c/d/e)` | 3 | 4 | 0 |
| `//a//b//c//d//e` | 8 | 9 | 1 |
| `/a[1]/b[1]/c[1]/d[1]/e` | 0 | 1 | 1 |
#315 removed one `nodesets.size == 1` optimization path. This pull
request will reduce the performance regression.
To reduce more sort calls, we need to mark nodeset ordering: introducing
`Nodeset = Struct.new(:nodes, :order)`
but IMO, it shouldn't be done now. If `sort` is optimized, one extra
sort won't be a problem. Optimizing `step` will be harder and the code
may be complicated.
### Note
This pull request will slightly add complexity and a risk to forgot
sorting the nodeset in some path.
The effect may seem drastic in some case for now, but it's just because
`sort` is currently worst `O(n^2)`. We can improve `sort` performance,
so there's an option to leave the sort strategy simple.
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

@tompng@naitoh