Remove unnecessary length checks + improved indexing of charCodeCache

[gustf]

removed unnecessary length checks + improved indexing of charCodeCache

[sindresorhus]

Can you explain your changes? Those things are there for a reason.

[gustf]

It is not necessary to add start here to the charCodeCache, as long as we do the same in the loop.

This make the bench ~4-5% faster on my cpu

[SamVerschueren]

Makes sense

[Yomguithereal]

Makes sense indeed. Can't remember why I did that thusly.

[gustf]

This check is taken care of in the prefix loop

[SamVerschueren]

Same here, the check at line 56 kicks in.

[Yomguithereal]

Once again this is an early exit and result should be benchmarked.

[gustf]

Here aLen is always smaller or equal than bLen due to the swap, i.e. always false (if not or equal to aLen)

[SamVerschueren]

So what you mean is, that if bLen is 0, then aLen must be 0 as well and the check at line 56 will kick in again and returns 0?

[gustf]

Yes :)

[Yomguithereal]

Yes, here I guess that since we swapped the strings, we can remove this condition since if both strings have 0 length, condition in line 7 will handle it.

[gustf]

This check is taken care of in the suffix loop

[SamVerschueren]

If aLen is equal to 0, the check at line 56 kicks in ending up in the same result. So yes, the result is the same. However, this is an early exit and thus you can argue that the faster we can exit and the less code we should execute, the better. But yes, this one can safely be removed.

[Yomguithereal]

That's right indeed. But I guess we should benchmark the change to see if this yields any positive result.

[gustf]

Yes, agreed it could be faster in some corner-cases

[sindresorhus]

See #8 and #7.

// @Yomguithereal

[gustf]

In general, I think removing or keeping the checks will not make a big difference performance-wise. Maybe keep some of them for clarity?

[Yomguithereal]

I guess the line 27 check can be removed. One solution would be to comment the checks clearly as early exits.

[gustf]

I will come back to this after work today

[Yomguithereal]

Can we speak about this also? You seem to have an edge there :). Haven't had much time to read your code but it seems you can speed up computations by performing several computations in one iteration of the loop? Is that it?

[gustf]

Can we speak about this also? You seem to have an edge there :). Haven't had much time to read your code but it seems you can speed up computations by performing several computations in one iteration of the loop? Is that it?

Yes thats it, I do loop unrolling on the outer loop and calculating 4 rows at a time in the inner loop :)

I did some reviewing regarding the checks I came to the following:

1. Check on line 23: Bail when a is an empty string
2. Check on line 27: Unnecessary check on bLen
3. Check on line 40: Bail when a as a whole is a suffix to b

In case 1 and 3 the checks makes it slightly faster, but makes all "fall through" cases slightly slower.
2 can be safely removed regardless.

What do you think?

[gustf]

Sorry but I might misunderstand what was decided, did you want me to change the PR to keep the checks?

[sindresorhus]

@Yomguithereal What do you think?

[Yomguithereal]

I don't remember clearly where we were but I guess the idea would be, concerning the remaining and maybe redundant checks, to only keep them if the benchmark differences are negligible and if it enhances readability of the algorithm so one does not have to understand that we don't make such check here because it can be found aggregated with another one somewhere else.

[Yomguithereal]

In case 1 and 3 the checks makes it slightly faster, but makes all "fall through" cases slightly slower.
2 can be safely removed regardless.

I agree that we can drop n°2.

For the other ones, I guess the "fall through" scenario is the most common one to be expected in real use cases. So @gustf would you advocate to remove them (by how much is that faster in your benchmarks)?

[Yomguithereal]

Afterwards, with your accord @gustf, I guess we could work on integrating and understanding your loop unrolling into this library?

On a side note, the Levensthein distance is often used to see if two strings are similar, which can be said if their distance is less than some threshold. In those cases, you will gain tremendous performance by using a capped Levenshtein distance function such as this one.

[gustf]

@Yomguithereal Sorry for late reply.

I haven't done any benchmarking with or without the checks but I think it will not really make a big difference. But since there will be less comparisons in the general case I think removing them makes sense.

And sure, we can do some work integrating the loop unrolling. I will have some more time in a few weeks. Very busy at work now before summer holidays.

[Yomguithereal]

Well I guess this can be merged then, @sindresorhus?