feat: bring up Array.shuffle #102
Conversation
There was a problem hiding this comment.
Great thanks @SukkaW for this!
There's just one thing, which unfortunately I didn't communicate well.
What's in master branch is considered deprecated, and rather in maintanance mode. Currently development centers around ext branch (from which ext package is published.
Would mind rebasing this PR against ext branch, and following conventions over there?
If you have any questions towards that, please ask (I know that docs may lack in placases).
Again, very sorry for confusion. I'll make ext branch default today, and add note in main README.md so it's no longer that confusing
|
Well, should I bring up |
@SukkaW No, it should be implemented in |
|
@medikoo The PR is ready for review now. Unit test cases and the docs are completed. |
|
It appears that the newest mocha (which utilises destructuring assignment syntax) is not compatible with Node.js 4 causes CI to fail. |
|
|
||
| while (sourceIndex) { | ||
| // eslint-disable-next-line no-bitwise | ||
| var randomIndex = (sourceIndex * random()) | 0; |
There was a problem hiding this comment.
With bitwise no more Math.ceil is required, which makes the shuffle even faster.
There was a problem hiding this comment.
How much do we gain, will it be observable faster? Do we have some numbers?
There was a problem hiding this comment.
https://repl.it/repls/CornyBrightConditions
Here is the benchmark.
There was a problem hiding this comment.
Thanks, still this benchmark doesn't seem to test purely Math.round vs bitwise stripping.
I've prepared one here: https://jsbench.me/mske89xfr3/1 and it shows negligible difference between both (note that both operations on its own are extremely fast)
It seems not harmful to stick to Math.round here, and it'll definitely be win for readability
There was a problem hiding this comment.
I have updated the original benchmark with Math.floor: https://repl.it/repls/CornyBrightConditions
It seems that Math.floor is still slower (At least it is slower in Node.js)
There was a problem hiding this comment.
This benchmark still involves a lot more code than it should. Technically it's purely about testing Math.floor(value) against value | 0.
Node.js uses same engine as Chrome, and difference that https://jsbench.me/mske89xfr3/1 shows over there is neglible.
Also this package is environment agnostic, meant to be used in any engine (e.g. in Firefox there's no speed difference between both)
There was a problem hiding this comment.
I'd use Math.floor now, and eventually revisit if we would have a real world report that it noticeable slows down performance somewhere.
|
|
||
| while (sourceIndex) { | ||
| // eslint-disable-next-line no-bitwise | ||
| var randomIndex = (sourceIndex * random()) | 0; |
There was a problem hiding this comment.
How much do we gain, will it be observable faster? Do we have some numbers?
|
|
||
| while (sourceIndex) { | ||
| // eslint-disable-next-line no-bitwise | ||
| var randomIndex = (sourceIndex * random()) | 0; |
There was a problem hiding this comment.
Thanks, still this benchmark doesn't seem to test purely Math.round vs bitwise stripping.
I've prepared one here: https://jsbench.me/mske89xfr3/1 and it shows negligible difference between both (note that both operations on its own are extremely fast)
It seems not harmful to stick to Math.round here, and it'll definitely be win for readability
The PR closes #100, bringing up a side-effect-free, and O(n) array shuffle function.
The unit test is not finished since
tadis not powerful enough to write a unit test forshuffle(Help is wanted).P.S. I want to adopt the unit test from https://github.com/philihp/fast-shuffle/blob/master/src/__tests__/index.test.js