Optimize the parse_attributes method to use `Source#match` to parse XML. #119

naitoh · 2024-03-12T00:23:28Z

Why?

Improve maintainability by consolidating processing into Source#match.

[Benchmark]

RUBYLIB= BUNDLER_ORIG_RUBYLIB= /Users/naitoh/.rbenv/versions/3.3.0/bin/ruby -v -S benchmark-driver /Users/naitoh/ghq/github.com/naitoh/rexml/benchmark/parse.yaml
ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [arm64-darwin22]
Calculating -------------------------------------
                         before       after  before(YJIT)  after(YJIT)
                 dom     10.891      10.622        16.356       17.403 i/s -     100.000 times in 9.182130s 9.414177s 6.113806s 5.746133s
                 sax     30.335      29.845        49.749       54.877 i/s -     100.000 times in 3.296483s 3.350595s 2.010071s 1.822259s
                pull     35.514      34.801        61.123       66.908 i/s -     100.000 times in 2.815793s 2.873484s 1.636041s 1.494591s
              stream     35.141      34.475        52.110       56.836 i/s -     100.000 times in 2.845646s 2.900638s 1.919017s 1.759456s

Comparison:
                              dom
         after(YJIT):        17.4 i/s
        before(YJIT):        16.4 i/s - 1.06x  slower
              before:        10.9 i/s - 1.60x  slower
               after:        10.6 i/s - 1.64x  slower

                              sax
         after(YJIT):        54.9 i/s
        before(YJIT):        49.7 i/s - 1.10x  slower
              before:        30.3 i/s - 1.81x  slower
               after:        29.8 i/s - 1.84x  slower

                             pull
         after(YJIT):        66.9 i/s
        before(YJIT):        61.1 i/s - 1.09x  slower
              before:        35.5 i/s - 1.88x  slower
               after:        34.8 i/s - 1.92x  slower

                           stream
         after(YJIT):        56.8 i/s
        before(YJIT):        52.1 i/s - 1.09x  slower
              before:        35.1 i/s - 1.62x  slower
               after:        34.5 i/s - 1.65x  slower

YJIT=ON : 1.06x - 1.10x faster
YJIT=OFF : 0.97x - 0.98x faster

kou · 2024-03-14T06:19:28Z

lib/rexml/parsers/baseparser.rb

-            msg = "Duplicate attribute #{name.inspect}"
-            raise REXML::ParseException.new(msg, @source, self)
-          end
+            if attributes[name]


Why did you change .has_key? to []?

I changed it to attributes[name] rather than attributes.has_key?(name) because it is faster to process and shorter to write.
Should I not have mixed it with this PR?

$ cat benchmark/attributes.yaml loop_count: 100000 contexts: - name: No YJIT prelude: | $LOAD_PATH.unshift(File.expand_path("lib")) require 'rexml' - name: YJIT prelude: | $LOAD_PATH.unshift(File.expand_path("lib")) require 'rexml' RubyVM::YJIT.enable prelude: | attributes = {} name = :a benchmark: 'attributes[name]' : attributes[name] 'attributes.has_key?(name)' : attributes.has_key?(name)

$ benchmark-driver benchmark/attributes.yaml Calculating ------------------------------------- No YJIT YJIT attributes[name] 51.680M 54.437M i/s - 100.000k times in 0.001935s 0.001837s attributes.has_key?(name) 46.019M 45.683M i/s - 100.000k times in 0.002173s 0.002189s Comparison: attributes[name] YJIT: 54436580.8 i/s No YJIT: 51679591.0 i/s - 1.05x slower attributes.has_key?(name) No YJIT: 46019327.8 i/s YJIT: 45682957.4 i/s - 1.01x slower

Oh... It's surprising...

Both of them just calls hash_stlike_lookup():

Hash#[]: https://github.com/ruby/ruby/blob/b4f3f3c1031cc9ef5c6741042236db497be6602b/hash.c#L2135-L2146
Hash#has_key?: https://github.com/ruby/ruby/blob/b4f3f3c1031cc9ef5c6741042236db497be6602b/hash.c#L3671-L3675

Could you split this to another PR for now? I want to take a look at it later.

test/parse/test_element.rb

test/test_core.rb

## Why? Improve maintainability by consolidating processing into `Source#match`. ## [Benchmark] ``` RUBYLIB= BUNDLER_ORIG_RUBYLIB= /Users/naitoh/.rbenv/versions/3.3.0/bin/ruby -v -S benchmark-driver /Users/naitoh/ghq/github.com/naitoh/rexml/benchmark/parse.yaml ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [arm64-darwin22] Calculating ------------------------------------- before after before(YJIT) after(YJIT) dom 10.891 10.622 16.356 17.403 i/s - 100.000 times in 9.182130s 9.414177s 6.113806s 5.746133s sax 30.335 29.845 49.749 54.877 i/s - 100.000 times in 3.296483s 3.350595s 2.010071s 1.822259s pull 35.514 34.801 61.123 66.908 i/s - 100.000 times in 2.815793s 2.873484s 1.636041s 1.494591s stream 35.141 34.475 52.110 56.836 i/s - 100.000 times in 2.845646s 2.900638s 1.919017s 1.759456s Comparison: dom after(YJIT): 17.4 i/s before(YJIT): 16.4 i/s - 1.06x slower before: 10.9 i/s - 1.60x slower after: 10.6 i/s - 1.64x slower sax after(YJIT): 54.9 i/s before(YJIT): 49.7 i/s - 1.10x slower before: 30.3 i/s - 1.81x slower after: 29.8 i/s - 1.84x slower pull after(YJIT): 66.9 i/s before(YJIT): 61.1 i/s - 1.09x slower before: 35.5 i/s - 1.88x slower after: 34.8 i/s - 1.92x slower stream after(YJIT): 56.8 i/s before(YJIT): 52.1 i/s - 1.09x slower before: 35.1 i/s - 1.62x slower after: 34.5 i/s - 1.65x slower ``` - YJIT=ON : 1.06x - 1.10x faster - YJIT=OFF : 0.97x - 0.98x faster

kou · 2024-03-18T14:30:50Z

Thanks!

## Why? `attributes[name]` is faster than `attribute.has_key?(name)` in Micro Benchmark. However, the Benchmark did not show a significant difference. Would like to merge if possible, how about it? See: ruby#119 (comment) ## Micro Benchmark ``` $ cat benchmark/attributes.yaml loop_count: 100000 contexts: - name: No YJIT prelude: | $LOAD_PATH.unshift(File.expand_path("lib")) require 'rexml' - name: YJIT prelude: | $LOAD_PATH.unshift(File.expand_path("lib")) require 'rexml' RubyVM::YJIT.enable prelude: | attributes = {} name = :a benchmark: 'attributes[name]' : attributes[name] 'attributes.has_key?(name)' : attributes.has_key?(name) ``` ``` $ benchmark-driver benchmark/attributes.yaml Calculating ------------------------------------- No YJIT YJIT attributes[name] 53.362M 53.562M i/s - 100.000k times in 0.001874s 0.001867s attributes.has_key?(name) 45.025M 45.005M i/s - 100.000k times in 0.002221s 0.002222s Comparison: attributes[name] YJIT: 53561863.6 i/s No YJIT: 53361791.1 i/s - 1.00x slower attributes.has_key?(name) No YJIT: 45024765.3 i/s YJIT: 45004502.0 i/s - 1.00x slower ``` ## Benchmark ``` RUBYLIB= BUNDLER_ORIG_RUBYLIB= /Users/naitoh/.rbenv/versions/3.3.0/bin/ruby -v -S benchmark-driver /Users/naitoh/ghq/github.com/naitoh/rexml/benchmark/parse.yaml ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [arm64-darwin22] Calculating ------------------------------------- before after before(YJIT) after(YJIT) dom 10.786 10.783 18.196 17.959 i/s - 100.000 times in 9.270908s 9.273657s 5.495854s 5.568326s sax 30.213 30.430 57.030 56.672 i/s - 100.000 times in 3.309845s 3.286240s 1.753459s 1.764551s pull 35.211 35.259 70.817 70.784 i/s - 100.000 times in 2.840056s 2.836136s 1.412098s 1.412754s stream 34.281 34.475 63.084 62.978 i/s - 100.000 times in 2.917067s 2.900689s 1.585196s 1.587860s Comparison: dom before(YJIT): 18.2 i/s after(YJIT): 18.0 i/s - 1.01x slower before: 10.8 i/s - 1.69x slower after: 10.8 i/s - 1.69x slower sax before(YJIT): 57.0 i/s after(YJIT): 56.7 i/s - 1.01x slower after: 30.4 i/s - 1.87x slower before: 30.2 i/s - 1.89x slower pull before(YJIT): 70.8 i/s after(YJIT): 70.8 i/s - 1.00x slower after: 35.3 i/s - 2.01x slower before: 35.2 i/s - 2.01x slower stream before(YJIT): 63.1 i/s after(YJIT): 63.0 i/s - 1.00x slower after: 34.5 i/s - 1.83x slower before: 34.3 i/s - 1.84x slower ``` - YJIT=ON : 0.98x - 1.00x faster - YJIT=OFF : 1.00x - 1.00x faster

## Why? `attributes[name]` is faster than `attribute.has_key?(name)` in Micro Benchmark. However, the Benchmark did not show a significant difference. Would like to merge if possible, how about it? See: #119 (comment) ## Micro Benchmark ``` $ cat benchmark/attributes.yaml loop_count: 100000 contexts: - name: No YJIT prelude: | $LOAD_PATH.unshift(File.expand_path("lib")) require 'rexml' - name: YJIT prelude: | $LOAD_PATH.unshift(File.expand_path("lib")) require 'rexml' RubyVM::YJIT.enable prelude: | attributes = {} name = :a benchmark: 'attributes[name]' : attributes[name] 'attributes.has_key?(name)' : attributes.has_key?(name) ``` ``` $ benchmark-driver benchmark/attributes.yaml Calculating ------------------------------------- No YJIT YJIT attributes[name] 53.362M 53.562M i/s - 100.000k times in 0.001874s 0.001867s attributes.has_key?(name) 45.025M 45.005M i/s - 100.000k times in 0.002221s 0.002222s Comparison: attributes[name] YJIT: 53561863.6 i/s No YJIT: 53361791.1 i/s - 1.00x slower attributes.has_key?(name) No YJIT: 45024765.3 i/s YJIT: 45004502.0 i/s - 1.00x slower ``` ## Benchmark ``` RUBYLIB= BUNDLER_ORIG_RUBYLIB= /Users/naitoh/.rbenv/versions/3.3.0/bin/ruby -v -S benchmark-driver /Users/naitoh/ghq/github.com/naitoh/rexml/benchmark/parse.yaml ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [arm64-darwin22] Calculating ------------------------------------- before after before(YJIT) after(YJIT) dom 10.786 10.783 18.196 17.959 i/s - 100.000 times in 9.270908s 9.273657s 5.495854s 5.568326s sax 30.213 30.430 57.030 56.672 i/s - 100.000 times in 3.309845s 3.286240s 1.753459s 1.764551s pull 35.211 35.259 70.817 70.784 i/s - 100.000 times in 2.840056s 2.836136s 1.412098s 1.412754s stream 34.281 34.475 63.084 62.978 i/s - 100.000 times in 2.917067s 2.900689s 1.585196s 1.587860s Comparison: dom before(YJIT): 18.2 i/s after(YJIT): 18.0 i/s - 1.01x slower before: 10.8 i/s - 1.69x slower after: 10.8 i/s - 1.69x slower sax before(YJIT): 57.0 i/s after(YJIT): 56.7 i/s - 1.01x slower after: 30.4 i/s - 1.87x slower before: 30.2 i/s - 1.89x slower pull before(YJIT): 70.8 i/s after(YJIT): 70.8 i/s - 1.00x slower after: 35.3 i/s - 2.01x slower before: 35.2 i/s - 2.01x slower stream before(YJIT): 63.1 i/s after(YJIT): 63.0 i/s - 1.00x slower after: 34.5 i/s - 1.83x slower before: 34.3 i/s - 1.84x slower ``` - YJIT=ON : 0.98x - 1.00x faster - YJIT=OFF : 1.00x - 1.00x faster

kou reviewed Mar 14, 2024

View reviewed changes

Add Missing attribute value end quote test case

7d00e7c

naitoh force-pushed the parse_attributes_string_scanner_style branch from cce06cd to 7137490 Compare March 15, 2024 13:04

naitoh requested a review from kou March 15, 2024 13:09

naitoh force-pushed the parse_attributes_string_scanner_style branch from 7137490 to a94c5b6 Compare March 15, 2024 14:41

kou merged commit 0496940 into ruby:master Mar 18, 2024
40 checks passed

naitoh deleted the parse_attributes_string_scanner_style branch March 18, 2024 14:41

naitoh mentioned this pull request Mar 20, 2024

Change attribute.has_key?(name) to attributes[name]. #121

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize the parse_attributes method to use `Source#match` to parse XML. #119

Optimize the parse_attributes method to use `Source#match` to parse XML. #119

naitoh commented Mar 12, 2024 •

edited

Loading

kou Mar 14, 2024

naitoh Mar 14, 2024

kou Mar 15, 2024

naitoh Mar 15, 2024

kou commented Mar 18, 2024

Optimize the parse_attributes method to use Source#match to parse XML. #119

Optimize the parse_attributes method to use Source#match to parse XML. #119

Conversation

naitoh commented Mar 12, 2024 • edited Loading

Why?

[Benchmark]

kou Mar 14, 2024

Choose a reason for hiding this comment

naitoh Mar 14, 2024

Choose a reason for hiding this comment

kou Mar 15, 2024

Choose a reason for hiding this comment

naitoh Mar 15, 2024

Choose a reason for hiding this comment

kou commented Mar 18, 2024

Optimize the parse_attributes method to use `Source#match` to parse XML. #119

Optimize the parse_attributes method to use `Source#match` to parse XML. #119

naitoh commented Mar 12, 2024 •

edited

Loading