Skip to content

Optimization of REXML::Document#add, #add_element#311

Open
naitoh wants to merge 1 commit intoruby:masterfrom
naitoh:fix_Document_add_add_element_performance
Open

Optimization of REXML::Document#add, #add_element#311
naitoh wants to merge 1 commit intoruby:masterfrom
naitoh:fix_Document_add_add_element_performance

Conversation

@naitoh
Copy link
Copy Markdown
Contributor

@naitoh naitoh commented May 5, 2026

Benchmark

$ benchmark-driver benchmark/parse_comment.yaml
                         before       after  before(YJIT)  after(YJIT)
           top_level    48.497k     54.645k        7.951k       8.288k i/s -     100.000 times in 0.002062s 0.001830s 0.012577s 0.012066s
          in_doctype    48.615k     50.994k        7.930k       7.820k i/s -     100.000 times in 0.002057s 0.001961s 0.012611s 0.012787s
       after_doctype    63.251k     64.061k        9.569k       9.434k i/s -     100.000 times in 0.001581s 0.001561s 0.010450s 0.010600s
       many_comments     67.653     611.015       117.191      929.290 i/s -     100.000 times in 1.478125s 0.163662s 0.853309s 0.107609s

Comparison:
                        top_level
               after:     54644.8 i/s
              before:     48496.6 i/s - 1.13x  slower
         after(YJIT):      8287.7 i/s - 6.59x  slower
        before(YJIT):      7951.0 i/s - 6.87x  slower

                       in_doctype
               after:     50994.4 i/s
              before:     48614.5 i/s - 1.05x  slower
        before(YJIT):      7929.6 i/s - 6.43x  slower
         after(YJIT):      7820.4 i/s - 6.52x  slower

                    after_doctype
               after:     64061.5 i/s
              before:     63251.1 i/s - 1.01x  slower
        before(YJIT):      9569.4 i/s - 6.69x  slower
         after(YJIT):      9434.0 i/s - 6.79x  slower

                    many_comments
         after(YJIT):       929.3 i/s
               after:       611.0 i/s - 1.52x  slower
        before(YJIT):       117.2 i/s - 7.93x  slower
              before:        67.7 i/s - 13.74x  slower
  • YJIT=ON : 0.98 - 7.93x faster
  • YJIT=OFF : 1.01 - 9.02x faster

## Benchmark
```
$ benchmark-driver benchmark/parse_comment.yaml
                         before       after  before(YJIT)  after(YJIT)
           top_level    48.497k     54.645k        7.951k       8.288k i/s -     100.000 times in 0.002062s 0.001830s 0.012577s 0.012066s
          in_doctype    48.615k     50.994k        7.930k       7.820k i/s -     100.000 times in 0.002057s 0.001961s 0.012611s 0.012787s
       after_doctype    63.251k     64.061k        9.569k       9.434k i/s -     100.000 times in 0.001581s 0.001561s 0.010450s 0.010600s
       many_comments     67.653     611.015       117.191      929.290 i/s -     100.000 times in 1.478125s 0.163662s 0.853309s 0.107609s

Comparison:
                        top_level
               after:     54644.8 i/s
              before:     48496.6 i/s - 1.13x  slower
         after(YJIT):      8287.7 i/s - 6.59x  slower
        before(YJIT):      7951.0 i/s - 6.87x  slower

                       in_doctype
               after:     50994.4 i/s
              before:     48614.5 i/s - 1.05x  slower
        before(YJIT):      7929.6 i/s - 6.43x  slower
         after(YJIT):      7820.4 i/s - 6.52x  slower

                    after_doctype
               after:     64061.5 i/s
              before:     63251.1 i/s - 1.01x  slower
        before(YJIT):      9569.4 i/s - 6.69x  slower
         after(YJIT):      9434.0 i/s - 6.79x  slower

                    many_comments
         after(YJIT):       929.3 i/s
               after:       611.0 i/s - 1.52x  slower
        before(YJIT):       117.2 i/s - 7.93x  slower
              before:        67.7 i/s - 13.74x  slower
```
- YJIT=ON : 0.98 - 7.93x faster
- YJIT=OFF : 1.01 - 9.02x faster
@naitoh naitoh requested a review from kou May 5, 2026 11:28
@kou kou requested a review from Copilot May 6, 2026 03:27
Copy link
Copy Markdown

Copilot AI left a comment

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 how REXML::Document enforces the “single root element” invariant in #add and #add_element, aiming to avoid unnecessary element-list work during parsing (notably when many non-element nodes like comments are added).

Changes:

  • Update REXML::Document#add to pre-check for an existing root element before delegating to super when adding an Element.
  • Update REXML::Document#add_element to raise immediately if a root element already exists, instead of checking after super.
  • Add benchmark coverage for a “many comments” parsing scenario and add tests around second-root-element rejection.

Reviewed changes

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

File Description
lib/rexml/document.rb Moves “second root element” checks to occur before super, avoiding post-add element scanning.
test/test_document.rb Adds tests covering second-root-element errors and allowing XMLDecl/DocType additions.
benchmark/parse_comment.yaml Adjusts benchmark inputs and adds a new many_comments benchmark case.

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

Comment thread test/test_document.rb
Comment on lines +326 to +349
assert_raise(RuntimeError, "attempted adding second root element to document") do
doc.add(REXML::Element.new("second"))
end
end

def test_append_operator_second_root_element_raises
doc = REXML::Document.new("<root/>")
assert_raise(RuntimeError, "attempted adding second root element to document") do
doc << REXML::Element.new("second")
end
end

def test_add_element_second_root_raises
doc = REXML::Document.new("<root/>")
assert_raise(RuntimeError, "attempted adding second root element to document") do
doc.add_element("second")
end
end

def test_add_element_with_element_second_root_raises
doc = REXML::Document.new("<root/>")
assert_raise(RuntimeError, "attempted adding second root element to document") do
doc.add_element(REXML::Element.new("second"))
end
Comment thread benchmark/parse_comment.yaml
Sign up for free to 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.

2 participants