Change namespace cache strategy by tompng · Pull Request #333 · ruby/rexml

tompng · 2026-06-14T17:45:45Z

Builds on top of #335

Even if namespace lookup is cached, namespace lookup was slow for deeply nested XML nodes.
Namespaces are cached in document, but document lookup from a deep node costs O(n).

We can't cache it in a document or root of the node, because we also need to cache document lookup result.
It's not good to cache to node itself, for maintainability reason, cache invalidation, etc.
In this PR, Hash for caching node's namespace/namespaces, and cached namespace attributes defined in attlist decl can be injected through method parameters.

Benchmark

irb> doc = REXML::Document.new('<a attr="1">'*1000+'</a>'*1000)
irb> measure
irb> REXML::XPath.match(doc, '//a');
# Master:                             0.2791398s
# Master(XPathParser#sort disabled):  0.1138055s
# This PR:                            0.1743505s
# This PR(XPathParser#sort disabled): 0.0046388s

Time consumption was: Scan: 0.0046s (1.8%), Sort: 0.17s (60%), Namespace lookup: 0.11s (38%). This PR removes the 38% cost.

Cache through parameters also speeds up bare namespace call on deeply nested element.

irb> doc = REXML::Document.new('<a>'*1000+'</a>'*1000)
irb> node = doc; node = node.first while node.first
irb> measure
irb> node.namespace
# Master:  0.079568s O(N^2)
# This PR: 0.003091s O(N)

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR refactors REXML namespace resolution to incorporate ATTLIST-declared xmlns values (and adds caching in the XPath parser), along with a new regression test that validates namespace precedence.

Changes:

Add support for applying ATTLIST-declared xmlns values into element/attribute namespace resolution.
Introduce per-parser caching for element namespace computations in XPathParser.
Add a new test for ATTLIST vs element-declared namespace precedence; remove the previous namespaces cache regression test.

Reviewed changes

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

Show a summary per file

File	Description
test/xpath/test_base.rb	Removes a regression test that previously exercised namespace cache invalidation behavior.
test/test_core.rb	Adds a test asserting ATTLIST-declared xmlns precedence vs inherited/locally-declared namespaces.
lib/rexml/xpath_parser.rb	Adds namespace caching and routes namespace comparisons through cached lookup helpers.
lib/rexml/element.rb	Refactors namespace calculation/lookup internals and integrates ATTLIST-declared namespaces.
lib/rexml/document.rb	Adds extraction of ATTLIST-declared xmlns mappings per element name.

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

tompng · 2026-06-14T18:01:41Z

@@ -2424,19 +2447,12 @@ def prefixes
    #   d.root.attributes.namespaces # => {"xmlns"=>"foo", "x"=>"bar", "y"=>"twee"}
    #
    def namespaces


This method is no longer called except from the added test.
We need to keep this if it's a public API, but if not, we can remove it (perhaps with deprecation first)

tompng · 2026-06-14T18:03:45Z

+      namespaces = namespaces.merge(attlist_namespaces) if attlist_namespaces
+      namespaces = namespaces.merge(own_namespaces) if own_namespaces.any?


There was a small bug in master: precedence of attlist attributes and own attirbutes was reversed. Test is added for this

Copilot

Pull request overview

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

Copilot

Pull request overview

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

+    def _attlist_mappings
+      mapping = {}
+      grep(AttlistDecl).each do |child|
+        raw_attributes = mapping[child.element_name] ||= {}
+        child.each do |key, val|
+          # First declaration wins
+          raw_attributes[key] = val unless raw_attributes.key? key
+        end
      end
-      return nil unless att_decl
-      att_decl[attribute]
+      mapping
    end


naitoh · 2026-07-03T00:07:08Z

-      else
-        match( path_stack, node )
-      end
+      @document = node.document


before (expect)

> doc = REXML::Document.new '<!DOCTYPE a [<!ATTLIST b xmlns:p CDATA "urn:x">]><a><p:c/></a>' > parser = REXML::XPathParser.new > parser.predicate("//p:c", doc) #=> [<p:c/>]

after (this PR)

> doc = REXML::Document.new '<!DOCTYPE a [<!ATTLIST b xmlns:p CDATA "urn:x">]><a><p:c/></a>' > parser = REXML::XPathParser.new > parser.predicate("//p:c", doc) # => []

Although @document is set in parse, it is not set in predicate or get_first, so @attlist_mappings becomes {} in those cases, and the xmlns declaration derived from ATTLIST does not take effect.

Fixed. But I can't confidently say that it works because predicate is not tested at all, and first (called from get_first) is an incomplete function.

# FIXME: This method is incomplete! def first( path_stack, node ) ... end

Maybe it's worth deleting them in a separate pull request.

Fixed.

Thanks

Maybe it's worth deleting them in a separate pull request.

We might want to consider deleting it.

Copilot

Pull request overview

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

Copilot

Pull request overview

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

naitoh · 2026-07-05T02:23:26Z



    def match(path_stack, node)
+      @document = node.document


When reusing an XPathParser, old cache entries remain.

before (expect)

> doc = REXML::Document.new("<a xmlns='u1'></a>") > parser = REXML::XPathParser.new > parser.namespaces = {"p" => "u1"} > parser.parse("//p:b", doc) => [] > doc.root => <a xmlns='u1'> ... </> > doc.root.delete_namespace => <a> ... </> > doc.root => <a> ... </> > parser.parse("//p:b", doc) => []

after (this PR)

> doc = REXML::Document.new("<a xmlns='u1'></a>") > parser = REXML::XPathParser.new > parser.namespaces = {"p" => "u1"} > parser.parse("//p:b", doc) => [] > doc.root.delete_namespace => <a> ... </> > doc.root => <a> ... </> > parser.parse("//p:b", doc) => []

Suggested change

@document = node.document

@document = node.document

@attlist_mappings = nil

@element_namespaces_cache = {}

naitoh · 2026-07-05T02:30:24Z

-      else
-        match( path_stack, node )
-      end
+      @document = node.document


Fixed.

Thanks

Maybe it's worth deleting them in a separate pull request.

We might want to consider deleting it.

Namespace calculation for each node can't be cached to document because document lookup is slow for deeply nested nodes. Change namespace cache strategy, inject cached hash as an argument to retrieve namespace/namespaces from XPath match operation and also for bare namespace call.

Co-authored-by: NAITOH Jun <naitoh@gmail.com>

…document

Copilot

Pull request overview

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

    def namespaces
-      namespaces_cache = document&.__send__(:namespaces_cache)
-      if namespaces_cache
-        namespaces_cache[self] ||= calculate_namespaces
-      else
-        calculate_namespaces
-      end
+      _calculate_namespaces
    end


    def get_first path, node
      path_stack = @parser.parse( path )
+      @document = node.document
      first( path_stack, node )
    end


    def match(path_stack, node)
+      @document = node.document
      nodeset = [node]
      result = expr(path_stack, nodeset)


    def attribute_of element, attribute
-      attribute_declarations_of(element)[attribute]
+      _attlist_mappings[element]&.[](attribute)
    end

-    def attribute_declarations_of(name)
-      raw_attributes = {}
-      decls = select do |child|
-        child.kind_of?(AttlistDecl) && child.element_name == name
-      end
-      decls.each do |child|
+    def _attlist_mappings # :nodoc:


Copilot AI review requested due to automatic review settings June 14, 2026 17:45

Copilot AI reviewed Jun 14, 2026

View reviewed changes

tompng force-pushed the xpath_namespace_lookup_cache branch from 8d141ec to 8a7840a Compare June 14, 2026 17:58

tompng commented Jun 14, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings June 15, 2026 10:50

tompng force-pushed the xpath_namespace_lookup_cache branch from 8a7840a to 95891d1 Compare June 15, 2026 10:50

Copilot AI reviewed Jun 15, 2026

View reviewed changes

Comment thread lib/rexml/xpath_parser.rb Outdated

Comment thread lib/rexml/xpath_parser.rb Outdated

Comment thread lib/rexml/element.rb Outdated

Comment thread lib/rexml/document.rb Outdated

Comment thread test/xpath/test_base.rb

tompng force-pushed the xpath_namespace_lookup_cache branch 2 times, most recently from 2856f4a to 58fa153 Compare June 16, 2026 18:06

Copilot AI review requested due to automatic review settings June 16, 2026 18:06

Copilot AI reviewed Jun 16, 2026

View reviewed changes

tompng force-pushed the xpath_namespace_lookup_cache branch from 58fa153 to 5d35322 Compare June 16, 2026 18:28

naitoh reviewed Jul 3, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 3, 2026 17:34

Copilot started reviewing on behalf of tompng July 3, 2026 17:34 View session

Copilot AI reviewed Jul 3, 2026

View reviewed changes

Comment thread lib/rexml/xpath_parser.rb Outdated

Comment thread lib/rexml/element.rb

tompng force-pushed the xpath_namespace_lookup_cache branch from 3e780d9 to a83e31a Compare July 3, 2026 17:45

naitoh reviewed Jul 4, 2026

View reviewed changes

Comment thread lib/rexml/attribute.rb

Copilot AI review requested due to automatic review settings July 4, 2026 15:39

tompng force-pushed the xpath_namespace_lookup_cache branch from a83e31a to 20c7dbc Compare July 4, 2026 15:39

Copilot started reviewing on behalf of tompng July 4, 2026 15:39 View session

Copilot AI reviewed Jul 4, 2026

View reviewed changes

Comment thread lib/rexml/xpath_parser.rb

Comment thread lib/rexml/xpath_parser.rb

Comment thread lib/rexml/element.rb

tompng force-pushed the xpath_namespace_lookup_cache branch from 20c7dbc to 032ab99 Compare July 4, 2026 17:51

naitoh reviewed Jul 5, 2026

View reviewed changes

tompng and others added 4 commits July 5, 2026 21:15

Apply suggestions from code review

f34d1cd

Co-authored-by: NAITOH Jun <naitoh@gmail.com>

Remove document's namespace cache API which is no longer used

e596e10

Update XPathParser#get_first and XPathParser#predicate to both setup …

b9dcebe

…document

Copilot AI review requested due to automatic review settings July 5, 2026 12:15

tompng force-pushed the xpath_namespace_lookup_cache branch from 032ab99 to b9dcebe Compare July 5, 2026 12:15

Copilot started reviewing on behalf of tompng July 5, 2026 12:16 View session

Copilot AI reviewed Jul 5, 2026

View reviewed changes

		namespaces = namespaces.merge(attlist_namespaces) if attlist_namespaces
		namespaces = namespaces.merge(own_namespaces) if own_namespaces.any?

Uh oh!

Conversation

tompng commented Jun 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tompng Jun 14, 2026

Choose a reason for hiding this comment

Uh oh!

tompng Jun 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

naitoh Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

tompng Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

naitoh Jul 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

naitoh Jul 5, 2026

Choose a reason for hiding this comment

Uh oh!

naitoh Jul 5, 2026

Choose a reason for hiding this comment

tompng commented Jun 14, 2026 •

edited

Loading