Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Mar 18, 2024

Conversation

naitoh
Copy link
Contributor

@naitoh naitoh commented Mar 12, 2024

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

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

Choose a reason for hiding this comment

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

Why did you change .has_key? to []?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I See.

test/parse/test_element.rb Outdated Show resolved Hide resolved
test/test_core.rb Outdated Show resolved Hide resolved
@naitoh naitoh force-pushed the parse_attributes_string_scanner_style branch from cce06cd to 7137490 Compare March 15, 2024 13:04
@naitoh naitoh requested a review from kou March 15, 2024 13:09
## 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
@naitoh naitoh force-pushed the parse_attributes_string_scanner_style branch from 7137490 to a94c5b6 Compare March 15, 2024 14:41
@kou kou merged commit 0496940 into ruby:master Mar 18, 2024
40 checks passed
@kou
Copy link
Member

kou commented Mar 18, 2024

Thanks!

@naitoh naitoh deleted the parse_attributes_string_scanner_style branch March 18, 2024 14:41
naitoh added a commit to naitoh/rexml that referenced this pull request Mar 20, 2024
## 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
kou pushed a commit that referenced this pull request Mar 22, 2024
## 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants