ngxs / store

🚀 NGXS - State Management for Angular
http://ngxs.io
MIT License
3.54k stars 405 forks source link

Operators array copy performance suggestion: Array.slice instead of [...spread] #1055

Closed GoGoris closed 5 years ago

GoGoris commented 5 years ago

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report  
[x] Performance issue
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => https://github.com/ngxs/store/blob/master/CONTRIBUTING.md
[ ] Other... Please describe:

Current behavior

Cloning an array inside the new operators is done with the spread operator. (removeItem, insertItem, updateItem) const clone = [...existing];

Expected behavior

const clone = existing.slice(0);

What is the motivation / use case for changing the behavior?

Compared to the spread operator, Array.prototype.slice() is up to 10 times faster in many browsers (FF, IE, Edge): https://jsperf.com/new-array-vs-splice-vs-slice/142

This difference can even increase for older browsers that do not support the spread operator natively and fallback to polyfills.

Environment


Libs:
- @angular/core version: N/A
- @ngxs/store version: 3.4.3


Browser:
- [ ] Chrome (desktop) version XX
- [ ] Chrome (Android) version XX
- [ ] Chrome (iOS) version XX
- [x] Firefox version 68.0b1
- [ ] Safari (desktop) version XX
- [ ] Safari (iOS) version XX
- [x] IE version 11
- [x] Edge version 41            
splincode commented 5 years ago

V8 v7.2 / Chrome 72 greatly improves the performance of spread elements when they occur at the front of the array literal, for example [...x] or [...x, 1, 2]. The improvement applies to spreading arrays, primitive strings, sets, maps keys, maps values, and — by extension — to Array.from(x).

GoGoris commented 5 years ago

The spread syntax in Chrome has the same performance as .splice() but in the other browsers (Firefox, Edge and IE) there is a significant difference. Edge and IE may be outdated but they are still used a lot. These browsers are slower at javascript to begin with, so they will slow down much faster when there are large arrays in the state. This will benefit websites that have a large state and the syntax is not really longer.

splincode commented 5 years ago

@markwhitfeld what do you think?

arturovt commented 5 years ago

I made some benchmarks. Array.prototype.slice turns out to be faster 2x faster in Mozilla and Safari (can't test other browsers tho).

In the Chrome slice and [...] have the same performance.

V8 has its own internal specification and implementation of the spread operator for arrays, except Mozilla 's SpiderMonkey compiler doesn't make any optimizations.

Also if we target es5 - spread is replaced with concat, concat is also slow in Mozilla/Safari.

I will a do a PR.

markwhitfeld commented 5 years ago

We will close this issue when the release goes out 👍 Aiming for this week unless any regressions pop up.

splincode commented 5 years ago

Released in 3.5.0