Implement Rabin-Karp algorithm for String#rindex

[makenowjust]

Reverse version of this.

Benchmark is here.

My result is:

$ crystal run --release rindex_bench.cr
For long string:
       rindex_old 258.31  (  3.87ms) (± 6.69%) 11.04× slower
rindex_rabin_karp   2.85k ( 350.8µs) (± 6.77%)       fastest
For short string:
       short_rindex_old   1.92M (520.84ns) (± 6.08%)  2.66× slower
short_rindex_rabin_karp   5.11M (195.87ns) (± 8.64%)       fastest

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

[drhuffman12]

Is it faster for short and long strings?

[makenowjust]

Old implementation is to scan string from head to end, and short string benchmark has no match but long string benchmark has match in middle of string, so long string benchmark is better score. Generally, the longer string length is, the better score.

[asterite]

Rabin-Karp is also faster for shorter strings. The old algorithm was very naive, for every char in the string check if there's a match from there on, so O(n*m), and Rabin-Karp will almost always be faster because of it's rolling hash algorithm.

Note that I knew the old algorithm was slow, it just wasn't a priority to improve it. There are many parts in the std that could use an optimization review.

@makenowjust By the way, there's also String#byte_index that currently has a very slow algorithm but could use Rabin-Karp (and it's easier, because it's byte index, not char index)

[makenowjust]

@asterite I have known it. I'll work tomorrow. Good night...

[drhuffman12]

Awesome! Crystal is already quite fast; but I won't complain if it gets even faster! :)

[bcardiff]

Besides the spec that was mysteriously removed, if you can squash the format tool commit to the parent one we will be fine to merge this as is.

Thanks for the performance addition and sending clear benchmarks with them 👍

[bcardiff]

why this spec was removed?

[makenowjust]

There is this in "with offset" section, but it has no offset option and it exists already in another line, so I thought it is just copy-and-paste mistaking.

[bcardiff]

Yeap, you are right.

[MaxLap]

I posted a possible improvement to the algorithm in the previous (for index) change #3873. I think it should be reviewed before merging this copy of it.

[makenowjust]

@MaxLap Your idea is awesome, but I think we cannot apply it to String#rindex.

[makenowjust]

@bcardiff Rebased

[MaxLap]

Ah, i see what you mean. A check could still be made to do another step unless the current byte is a valid starting one, but there may be less to gain here. You could still try it if you want.

if (byte & 0x80) == 0 || (byte & 0x40) != 0 is the same as if (byte & 0xC0) != 0x80. But maybe the compiler handles that one itself.

[sdogruyol]

Keep up the great work people 🎉 😍

[makenowjust]

@MaxLap Thanks. I applied some optimizations.

[bcardiff]

Thanks @makenowjust and @MaxLap ! ⚡️ ⚡️

[kostya]

little changed behavior after this:

0.20.4
crystal eval 'p "".includes?("")'
false

0.20.5
crystal eval 'p "".includes?("")'
true

not sure what is really correct.

[MaxLap]

Really? for me, on 0.20.5, I still get true O_o

[kostya]

ops i miss versions, fixed
so, true, i think correct, ok

[MaxLap]

True would be the same result as "".include?("") in ruby.

It may be interesting to add this edge case to the specs.