Skip to content

faster String#strip #2815

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

faster String#strip #2815

wants to merge 1 commit into from

Conversation

ahorek
Copy link
Contributor

@ahorek ahorek commented Jan 4, 2020

an experiment to make String#strip faster

                             master       patch
       small_chars.strip    12.453M     13.319M i/s -     29.593M times in 2.376336s 2.221917s
       large_chars.strip    10.916M     11.894M i/s -     29.455M times in 2.698388s 2.476395s
 small_left_spaces.strip     7.970M      8.808M i/s -     20.257M times in 2.541616s 2.299741s
 large_left_spaces.strip    94.021k    750.828k i/s -    281.753k times in 2.996706s 0.375256s
small_right_spaces.strip     8.236M      8.984M i/s -     21.576M times in 2.619551s 2.401707s
large_right_spaces.strip    90.280k    584.162k i/s -    283.410k times in 3.139241s 0.485157s
 small_both_spaces.strip     8.217M      9.050M i/s -     20.305M times in 2.471132s 2.243693s
 large_both_spaces.strip   106.757k    662.499k i/s -    311.799k times in 2.920633s 0.470641s

Comparison:
                    small_chars.strip
                   patch:  13318561.0 i/s
                  master:  12453094.6 i/s - 1.07x  slower

                    large_chars.strip
                   patch:  11894264.3 i/s
                  master:  10915735.1 i/s - 1.09x  slower

              small_left_spaces.strip
                   patch:   8808228.0 i/s
                  master:   7969984.5 i/s - 1.11x  slower

              large_left_spaces.strip
                   patch:    750828.4 i/s
                  master:     94020.9 i/s - 7.99x  slower

             small_right_spaces.strip
                   patch:   8983524.9 i/s
                  master:   8236446.6 i/s - 1.09x  slower

             large_right_spaces.strip
                   patch:    584161.9 i/s
                  master:     90279.8 i/s - 6.47x  slower

              small_both_spaces.strip
                   patch:   9049741.8 i/s
                  master:   8216819.6 i/s - 1.10x  slower

              large_both_spaces.strip
                   patch:    662498.9 i/s
                  master:    106757.3 i/s - 6.21x  slower

@ahorek ahorek changed the title WIP: fast strip faster String#strip Jan 4, 2020
@shyouhei
Copy link
Member

shyouhei commented Jan 5, 2020

Promising result! However, file arrangement seems strange. Headers under include/ are to be installed by make install. They affect extension libraries, not only ruby itself. I don't see any benefits for that so far. Can you tell us the reason you placed simd_strip.h there if any? If you have no string reason, let me advice you to choose a different place.

@ahorek
Copy link
Contributor Author

ahorek commented Jan 5, 2020

hi @shyouhei
no reason, please advise an appropriate place!

There're more opportunities where we could make core operations significantly faster, but at the cost of a more complicated code. There has to be a duplication if we want a fast variant for other architectures like ARM. In this case, the original (slow) version is used.
Unfortunately, auto-vectorization usually still can't beat handwritten code in performance and readability.

I would like to know if those optimizations are acceptable to the ruby-core?

btw I took inspiration from @casperisfine 's work at Shopify#2 (fastvalidate) which is more important for real-world performance.

@casperisfine
Copy link
Contributor

I would like to know if those optimizations are acceptable to the ruby-core?

I'd like to know this as well, that's why I have done a private PR first. I might open a ticket on Redmine to ask wether SIMD usage would be acceptable.

@shyouhei
Copy link
Member

shyouhei commented Jan 8, 2020

Yes, @casperisfine could you to open a ticket, so that I can 👍 there as well? I believe those vectorization techniques are worth added into the core. Of course there must be cost versus merit debates so discussions should be there for each of them but in general, I see no reason to reject those speedups.

@ahorek for simd_strip.h's place, I recommend you to put it under internal/.

@casperisfine
Copy link
Contributor

@shyouhei done: https://bugs.ruby-lang.org/issues/16487

Copy link
Member

@shyouhei shyouhei left a comment

Choose a reason for hiding this comment

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

Though we need to wait for the redmine discussions, the patch itself nonetheless looks good to me now. 👍

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.

3 participants