dcodeIO / bcrypt.js

Optimized bcrypt in plain JavaScript with zero dependencies.
Other
3.47k stars 264 forks source link

Change of method of concatination #73

Open Ruffio opened 6 years ago

Ruffio commented 6 years ago

Looking thru the code, I can see (naturally) multiple places with '.push' Especially in Chrome and Edge browsers there can be gained a lot if used another method.

Try to have a look at this test: https://jsperf.com/join-concat/2

I ran this test with (Chrome, IE, Edge and Firefox) on Windows 10 and here you can see that big differences: https://github.com/ricmoo/aes-js/issues/33#issuecomment-332121243

dcodeIO commented 6 years ago

There are several places, most likely, that could use a fixed-size array instead. That should be even better than string concatenation. Hasn't been a priority so far.

Ruffio commented 6 years ago

I'm not that convinced that fixed array is faster, but maybe that could just be my lack of skills:

image

dcodeIO commented 6 years ago
for (var res = new Array(10000), i = 0; i < 10000; i++) {
  res[i] = 'a';
}
res = res.join("");
Ruffio commented 6 years ago

Better, but still way slower than 'pure string concatenation': image

Ruffio commented 6 years ago

It is the last joining that kills the usage of array:

image

laggingreflex commented 6 years ago

res.concat('a') is fastest. https://jsbench.me/d8j89wmlki/1

Ruffio commented 6 years ago

Indeed: image

I ran the above tests 3 times and all showed the above.

IMHO the conclusion is that if the end result is a string use .concat, if the end result is an array, use the 'fixed array' with the size initially set.

vonagam commented 6 years ago

res.concat('a') does not mutates original string. It returns new one (just like concat for arrays). So it should be res = res.concat('a').

laggingreflex commented 6 years ago

@vonagam yeah you're right, my bad. It actually seems to be the slowest.

Ruffio commented 5 years ago

Just ran a new benchmark: image

When using res.join it is absolutely the slowest of them all and since we are working with strings then we have to use res.join.

Looks like += and .concat are the same. As I see it we should just switch to +=. That is simple and efficient. @dcodeIO What do you say? Will you review and accept a PR where += is used?