Replaced array.join('') to string += concat.

[ghost]

array.join is slower than += in modern javascript engines including V8.

See SO question - Why is string concatenation faster than array join?

[joshball]

Would you mind writing some tests (for this library) demonstrating this in the common cases used by bcrypt? For instance, I ran this (node 0.10.33, windows) and it was a toss up.

[ghost]

That test script seems not fair, as 'String array join()' operation needs a array per string.

Fair test script

Test env: Windows 8.1 64-bit,

Fair test result:

> node --version
v0.12.4

> node string-concat-join.js 60
60
Plain concatenation x 1,677,372 ops/sec ±3.17% (79 runs sampled
String array join() x 1,156,274 ops/sec ±2.34% (76 runs sampled
Fastest is Plain concatenation

> node string-concat-join.js 5328
5328
Plain concatenation x 17,719 ops/sec ±3.23% (75 runs sampled)
String array join() x 11,781 ops/sec ±1.64% (93 runs sampled)
Fastest is Plain concatenation

> node string-concat-join.js 10
10
Plain concatenation x 8,643,067 ops/sec ±1.94% (80 runs sampled)
String array join() x 5,700,618 ops/sec ±2.35% (86 runs sampled)
Fastest is Plain concatenation

[joshball]

That looks good, but what about a perf test for bcrypt lib itself. Add it to the PR showing the current perf and then run it with your PR to show the perf increase.

On May 27, 2015, at 7:50 PM, DongYun Kang notifications@github.com wrote:

That test script seems not fair, as 'String array join()' operation needs a array per string.

Fair test script

Test env: Windows 8.1 64-bit,

Fair test result:

node --version v0.12.4

node string-concat-join.js 60 60 Plain concatenation x 1,677,372 ops/sec ±3.17% (79 runs sampled String array join() x 1,156,274 ops/sec ±2.34% (76 runs sampled Fastest is Plain concatenation

node string-concat-join.js 5328 5328 Plain concatenation x 17,719 ops/sec ±3.23% (75 runs sampled) String array join() x 11,781 ops/sec ±1.64% (93 runs sampled) Fastest is Plain concatenation

node string-concat-join.js 10 10 Plain concatenation x 8,643,067 ops/sec ±1.94% (80 runs sampled) String array join() x 5,700,618 ops/sec ±2.35% (86 runs sampled) Fastest is Plain concatenation — Reply to this email directly or view it on GitHub.

[ghost]

It seem it doesn't affect performance, so there's no reason to use array.join for performance. And string concatenation is much better for code readability.

Test gist

> node index
Plain concatenation x 1.96 ops/sec ±0.60% (9 runs sampled
String array join() x 1.95 ops/sec ±0.57% (9 runs sampled
Fastest is Plain concatenation,String array join()

> node index
Plain concatenation x 1.96 ops/sec ±0.88% (9 runs sampled
String array join() x 1.95 ops/sec ±1.01% (9 runs sampled
Fastest is Plain concatenation,String array join()

[joshball]

Nice work! Great test. Thanks for following through. I certainly learned something.