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

Implement Rabin-Karp algorithm for String#index #3873

Merged

Conversation

makenowjust
Copy link
Contributor

Benchmark is here.

My result is:

$ crystal run --release index_bench.cr
       index_old 473.18  (  2.11ms) (± 8.92%)  2.72× slower
index_rabin_karp   1.29k (777.94µs) (±11.77%)       fastest

(I recommend you run this benchmark on your environment ;)

@sdogruyol
Copy link
Member

Wow, this is amazing! Good job @makenowjust 👍

@yxhuvud
Copy link
Contributor

yxhuvud commented Jan 11, 2017

One thing that would be interesting to see is how it compares for smaller strings. The setup cost for rabin-karp looks to be a lot higher - perhaps it would be a good idea to do approach it similar to sorting, ie if the size is less than a threshold, use index_old. But only benchmarking can show that.

@sdogruyol
Copy link
Member

I've updated the benchmark to include a short source with same target https://gist.github.com/sdogruyol/968802c00924528a0b37a73f4f3b4022

             index_old 706.59  (  1.42ms) (± 6.79%) 9894.91× slower
      index_rabin_karp   1.58k (634.75µs) (± 3.35%) 4437.92× slower
           index_short   3.62M ( 276.3ns) (± 5.36%)    1.93× slower
index_short_rabin_karp   6.99M (143.03ns) (± 7.61%)         fastest

It shows that Rabin Karp is still 2x faster.

@makenowjust
Copy link
Contributor Author

@sdogruyol Thanks!

@bcardiff bcardiff merged commit 0bb5a81 into crystal-lang:master Jan 11, 2017
@bcardiff
Copy link
Member

Thanks @makenowjust ⚡️ !

@bcardiff bcardiff added this to the Next milestone Jan 11, 2017
@makenowjust makenowjust deleted the feature/string/index-rabin-karp branch January 11, 2017 13:29
@makenowjust
Copy link
Contributor Author

@bcardiff Thanks too for fast response ⚡️

@MaxLap
Copy link
Contributor

MaxLap commented Jan 13, 2017

This seems to be a relatively good improvement to the algorithm. I didn't verify that the specs pass, so maybe something is wrong it. I'm suprised by how much of an improvement this shows, considering that ascii text is the case that should be gaining the less benefit.

I guess the if (byte & 0x80) == 0 || (byte & 0x40) != 0 check is pricier than it looks.

I'm basically just reducing the number of bigloop iterations needed by doing multibytes jumps, the same way it is done when finding the offset.

https://gist.github.com/MaxLap/0ab65d0e1466a7ba1107f66d0983be52

              index_old 265.23  (  3.77ms) (± 2.75%) 20941.18× slower
       index_rabin_karp 852.97  (  1.17ms) (± 5.87%)  6511.56× slower
      index_rabin_karp2   1.15k (873.18µs) (± 7.78%)  4849.77× slower
            index_short   1.96M (509.09ns) (±13.31%)     2.83× slower
 index_short_rabin_karp   3.96M (252.25ns) (±16.32%)     1.40× slower
index_short_rabin_karp2   5.55M (180.05ns) (±17.73%)          fastest

@makenowjust
Copy link
Contributor Author

makenowjust commented Jan 13, 2017

@MaxLap Your CPU is...? I think your CPU's no zero comparison is slower than mine. (My CPU is 1.6 GHz Intel Core i5 on MacBook Air, below is my result)

              index_old 604.03  (  1.66ms) (± 2.83%) 10648.60× slower
       index_rabin_karp   1.42k (706.58µs) (± 4.68%)  4544.78× slower
      index_rabin_karp2   1.46k (683.66µs) (± 4.03%)  4397.35× slower
        short_index_old   3.33M (299.97ns) (± 6.84%)     1.93× slower
 short_index_rabin_karp   6.09M (164.22ns) (± 9.57%)     1.06× slower
short_index_rabin_karp2   6.43M (155.47ns) (± 9.18%)          fastest

Of course, it is efficient for multi-byte text. And I rewrite your code by using macros to reduce code duplication (here).

@MaxLap
Copy link
Contributor

MaxLap commented Jan 13, 2017

I'm on Windows. So the benchmark was through a VM of Ubuntu 12.04 32bit with 2 cores. I7-3517U at 2.4GHz

I tried on another VM, Ubuntu 16.04 64bit. The improvement is less (but still higher than yours)

A bit sad that it's less impressive... Oh well, it's still an improvement!

@akzhan
Copy link
Contributor

akzhan commented Jun 23, 2017

btw, @makenowjust, are you plan to merge @MaxLap hint?

@makenowjust
Copy link
Contributor Author

@akzhan already implemented in #3876.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants