Optimize BaseParser#unnormalize method #158

naitoh · 2024-06-23T15:02:27Z

Benchmark

RUBYLIB= BUNDLER_ORIG_RUBYLIB= /Users/naitoh/.rbenv/versions/3.3.3/bin/ruby -v -S benchmark-driver /Users/naitoh/ghq/github.com/naitoh/rexml/benchmark/parse.yaml
ruby 3.3.3 (2024-06-12 revision f1c7b6f435) [arm64-darwin22]
Calculating -------------------------------------
                         before       after  before(YJIT)  after(YJIT)
                 dom     17.704      18.106        34.215       33.806 i/s -     100.000 times in 5.648398s 5.523110s 2.922698s 2.958036s
                 sax     25.664      25.302        48.429       48.602 i/s -     100.000 times in 3.896488s 3.952289s 2.064859s 2.057537s
                pull     28.966      29.215        61.710       62.068 i/s -     100.000 times in 3.452275s 3.422901s 1.620480s 1.611129s
              stream     28.291      28.426        53.860       55.548 i/s -     100.000 times in 3.534716s 3.517884s 1.856667s 1.800247s

Comparison:
                              dom
        before(YJIT):        34.2 i/s
         after(YJIT):        33.8 i/s - 1.01x  slower
               after:        18.1 i/s - 1.89x  slower
              before:        17.7 i/s - 1.93x  slower

                              sax
         after(YJIT):        48.6 i/s
        before(YJIT):        48.4 i/s - 1.00x  slower
              before:        25.7 i/s - 1.89x  slower
               after:        25.3 i/s - 1.92x  slower

                             pull
         after(YJIT):        62.1 i/s
        before(YJIT):        61.7 i/s - 1.01x  slower
               after:        29.2 i/s - 2.12x  slower
              before:        29.0 i/s - 2.14x  slower

                           stream
         after(YJIT):        55.5 i/s
        before(YJIT):        53.9 i/s - 1.03x  slower
               after:        28.4 i/s - 1.95x  slower
              before:        28.3 i/s - 1.96x  slower

YJIT=ON : 1.00x - 1.03x faster
YJIT=OFF : 0.98x - 1.02x faster

kou · 2024-06-23T20:23:23Z

lib/rexml/parsers/baseparser.rb

      # Unescapes all possible entities
-      def unnormalize( string, entities=nil, filter=nil )
-        rv = string.gsub( /\r\n?/, "\n" )
+      def unnormalize( rv, entities=nil, filter=nil )


Could you avoid using rv name for parameter name?
rv is return value. It's not suitable for parameter name.

kou · 2024-06-23T20:24:24Z

test/test_pullparser.rb

+      pp = REXML::Parsers::PullParser.new( source )
+      el_name = ''


Let's use better name in new tests:

Suggested change

pp = REXML::Parsers::PullParser.new( source )

el_name = ''

parser = REXML::Parsers::PullParser.new( source )

element_name = ''

kou · 2024-06-24T21:15:47Z

lib/rexml/parsers/baseparser.rb


      # Unescapes all possible entities
      def unnormalize( string, entities=nil, filter=nil )
-        rv = string.gsub( /\r\n?/, "\n" )


It seems that this is a backward incompatible change.
Do we have any existing test that string includes \r\n as new line?

There is no existing test.
I removed it because I thought it was unnecessary for the unnormalize process, but it breaks compatibility.
I will remove this change.
Sorry.

## Benchmark ``` RUBYLIB= BUNDLER_ORIG_RUBYLIB= /Users/naitoh/.rbenv/versions/3.3.3/bin/ruby -v -S benchmark-driver /Users/naitoh/ghq/github.com/naitoh/rexml/benchmark/parse.yaml ruby 3.3.3 (2024-06-12 revision f1c7b6f435) [arm64-darwin22] Calculating ------------------------------------- before after before(YJIT) after(YJIT) dom 17.704 18.106 34.215 33.806 i/s - 100.000 times in 5.648398s 5.523110s 2.922698s 2.958036s sax 25.664 25.302 48.429 48.602 i/s - 100.000 times in 3.896488s 3.952289s 2.064859s 2.057537s pull 28.966 29.215 61.710 62.068 i/s - 100.000 times in 3.452275s 3.422901s 1.620480s 1.611129s stream 28.291 28.426 53.860 55.548 i/s - 100.000 times in 3.534716s 3.517884s 1.856667s 1.800247s Comparison: dom before(YJIT): 34.2 i/s after(YJIT): 33.8 i/s - 1.01x slower after: 18.1 i/s - 1.89x slower before: 17.7 i/s - 1.93x slower sax after(YJIT): 48.6 i/s before(YJIT): 48.4 i/s - 1.00x slower before: 25.7 i/s - 1.89x slower after: 25.3 i/s - 1.92x slower pull after(YJIT): 62.1 i/s before(YJIT): 61.7 i/s - 1.01x slower after: 29.2 i/s - 2.12x slower before: 29.0 i/s - 2.14x slower stream after(YJIT): 55.5 i/s before(YJIT): 53.9 i/s - 1.03x slower after: 28.4 i/s - 1.95x slower before: 28.3 i/s - 1.96x slower ``` - YJIT=ON : 1.00x - 1.03x faster - YJIT=OFF : 0.98x - 1.02x faster

naitoh · 2024-06-24T22:44:02Z

After removing rv = string.gsub( /\r\n?/,"\n"), the effect is not so great...

kou · 2024-06-25T00:08:25Z

Thanks.

if string.include?("\r")
  rv = string.gsub( /\r\n?/, "\n" )
else
  rv = string.dup
end

may be faster.

…ly when "\r\n" is included ## Why? See: ruby#158 (comment) ## Benchmark ``` RUBYLIB= BUNDLER_ORIG_RUBYLIB= /Users/naitoh/.rbenv/versions/3.3.3/bin/ruby -v -S benchmark-driver /Users/naitoh/ghq/github.com/naitoh/rexml/benchmark/parse.yaml ruby 3.3.3 (2024-06-12 revision f1c7b6f435) [arm64-darwin22] Calculating ------------------------------------- before after before(YJIT) after(YJIT) dom 17.674 17.567 32.759 32.316 i/s - 100.000 times in 5.657973s 5.692371s 3.052595s 3.094448s sax 25.261 25.377 48.889 49.911 i/s - 100.000 times in 3.958626s 3.940640s 2.045460s 2.003575s pull 28.968 29.121 61.584 61.774 i/s - 100.000 times in 3.452132s 3.433967s 1.623789s 1.618809s stream 28.395 28.803 55.289 57.970 i/s - 100.000 times in 3.521761s 3.471812s 1.808673s 1.725029s Comparison: dom before(YJIT): 32.8 i/s after(YJIT): 32.3 i/s - 1.01x slower before: 17.7 i/s - 1.85x slower after: 17.6 i/s - 1.86x slower sax after(YJIT): 49.9 i/s before(YJIT): 48.9 i/s - 1.02x slower after: 25.4 i/s - 1.97x slower before: 25.3 i/s - 1.98x slower pull after(YJIT): 61.8 i/s before(YJIT): 61.6 i/s - 1.00x slower after: 29.1 i/s - 2.12x slower before: 29.0 i/s - 2.13x slower stream after(YJIT): 58.0 i/s before(YJIT): 55.3 i/s - 1.05x slower after: 28.8 i/s - 2.01x slower before: 28.4 i/s - 2.04x slower ``` - YJIT=ON : 0.98x - 1.05x faster - YJIT=OFF : 0.98x - 1.02x faster --------- Co-authored-by: Sutou Kouhei <[email protected]>

…ly when "\r\n" is included (#160) ## Why? See: #158 (comment) ## Benchmark ``` RUBYLIB= BUNDLER_ORIG_RUBYLIB= /Users/naitoh/.rbenv/versions/3.3.3/bin/ruby -v -S benchmark-driver /Users/naitoh/ghq/github.com/naitoh/rexml/benchmark/parse.yaml ruby 3.3.3 (2024-06-12 revision f1c7b6f435) [arm64-darwin22] Calculating ------------------------------------- before after before(YJIT) after(YJIT) dom 17.674 17.567 32.759 32.316 i/s - 100.000 times in 5.657973s 5.692371s 3.052595s 3.094448s sax 25.261 25.377 48.889 49.911 i/s - 100.000 times in 3.958626s 3.940640s 2.045460s 2.003575s pull 28.968 29.121 61.584 61.774 i/s - 100.000 times in 3.452132s 3.433967s 1.623789s 1.618809s stream 28.395 28.803 55.289 57.970 i/s - 100.000 times in 3.521761s 3.471812s 1.808673s 1.725029s Comparison: dom before(YJIT): 32.8 i/s after(YJIT): 32.3 i/s - 1.01x slower before: 17.7 i/s - 1.85x slower after: 17.6 i/s - 1.86x slower sax after(YJIT): 49.9 i/s before(YJIT): 48.9 i/s - 1.02x slower after: 25.4 i/s - 1.97x slower before: 25.3 i/s - 1.98x slower pull after(YJIT): 61.8 i/s before(YJIT): 61.6 i/s - 1.00x slower after: 29.1 i/s - 2.12x slower before: 29.0 i/s - 2.13x slower stream after(YJIT): 58.0 i/s before(YJIT): 55.3 i/s - 1.05x slower after: 28.8 i/s - 2.01x slower before: 28.4 i/s - 2.04x slower ``` - YJIT=ON : 0.98x - 1.05x faster - YJIT=OFF : 0.98x - 1.02x faster --------- Co-authored-by: Sutou Kouhei <[email protected]>

kou reviewed Jun 23, 2024

View reviewed changes

Add a test for BaseParser#unnormalize

683e156

naitoh force-pushed the optimize_base_parser_unnormalize branch from e1f4758 to 7727211 Compare June 23, 2024 21:06

naitoh requested a review from kou June 23, 2024 21:11

kou reviewed Jun 24, 2024

View reviewed changes

naitoh force-pushed the optimize_base_parser_unnormalize branch from 7727211 to 2a7f8a4 Compare June 24, 2024 22:37

naitoh requested a review from kou June 24, 2024 22:44

kou merged commit a579730 into ruby:master Jun 25, 2024

naitoh deleted the optimize_base_parser_unnormalize branch June 25, 2024 00:20

naitoh mentioned this pull request Jun 25, 2024

Optimize BaseParser#unnormalize method to replace "\r\n" with "\n" only when "\r\n" is included #160

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Optimize BaseParser#unnormalize method #158

Optimize BaseParser#unnormalize method #158

Uh oh!

naitoh commented Jun 23, 2024 •

edited

Loading

Uh oh!

kou Jun 23, 2024

Uh oh!

naitoh Jun 23, 2024

Uh oh!

kou Jun 23, 2024

Uh oh!

naitoh Jun 23, 2024

Uh oh!

kou Jun 24, 2024

Uh oh!

naitoh Jun 24, 2024

Uh oh!

naitoh commented Jun 24, 2024

Uh oh!

kou commented Jun 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Optimize BaseParser#unnormalize method #158

Optimize BaseParser#unnormalize method #158

Uh oh!

Conversation

naitoh commented Jun 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmark

Uh oh!

kou Jun 23, 2024

Choose a reason for hiding this comment

Uh oh!

naitoh Jun 23, 2024

Choose a reason for hiding this comment

Uh oh!

kou Jun 23, 2024

Choose a reason for hiding this comment

Uh oh!

naitoh Jun 23, 2024

Choose a reason for hiding this comment

Uh oh!

kou Jun 24, 2024

Choose a reason for hiding this comment

Uh oh!

naitoh Jun 24, 2024

Choose a reason for hiding this comment

Uh oh!

naitoh commented Jun 24, 2024

Uh oh!

kou commented Jun 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

naitoh commented Jun 23, 2024 •

edited

Loading