bcrypt.js icon indicating copy to clipboard operation
bcrypt.js copied to clipboard

Change of method of concatination

Open Ruffio opened this issue 7 years ago • 10 comments

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

Ruffio avatar Sep 27 '17 11:09 Ruffio

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.

dcodeIO avatar Sep 27 '17 12:09 dcodeIO

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

image

Ruffio avatar Sep 27 '17 12:09 Ruffio

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

dcodeIO avatar Sep 27 '17 12:09 dcodeIO

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

Ruffio avatar Sep 27 '17 12:09 Ruffio

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

image

Ruffio avatar Sep 27 '17 12:09 Ruffio

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

laggingreflex avatar Oct 02 '17 08:10 laggingreflex

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.

Ruffio avatar Oct 02 '17 08:10 Ruffio

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').

vonagam avatar Oct 05 '17 21:10 vonagam

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

laggingreflex avatar Oct 06 '17 00:10 laggingreflex

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?

Ruffio avatar Aug 25 '18 11:08 Ruffio