perf: remove object spreading

[baldurh]

Description

I replaced all instances of the object spread operator with Object.assign. The results are quite remarkable:

Benchmarks in ms. 100,000 iterations

	before	after	change
responsive	1652.000	1104.215	-33.16%
responsive with fixed height	1373.051	886.121	-35.46%
responsive with aspect ratio	1862.155	1267.419	-31.94%
fixed width	1347.304	867.349	-35.62%
fixed width with quality	1420.417	908.058	-36.07%
fixed width with dpr quality disabled	1305.567	872.474	-33.17%
fixed width with aspect ratio	1326.964	861.080	-35.11%

Flamegraph before
Flamegraph after

[frederickfogerty]

We'd rather not have non-package-related exclusions checked into source control. Directories and files can be ignored locally (and not checked into source control) by moving this line to .git/info/exclude. This command will do it for you: echo .clinic >> .git/info/exclude

[baldurh]

Roger. Fixed!

[frederickfogerty]

Thanks again @baldurh! LGTM once that .gitignore change is fixed 👍

[frederickfogerty]

LGTM 👍

[frederickfogerty]

Food for thought here - in Chrome 76+, spread is actually faster than Object.assign 🤷‍♀️

Source: https://jsperf.com/object-assign-vs-spreading

[baldurh]

Food for thought here - in Chrome 76+, spread is actually faster than Object.assign 🤷‍♀️

Wow that’s super interesting. We definitely have to take that into account. I ran the benchmarks on node 12 and Object.assign is still faster than object spreading. Chrome 76 uses V8 7.6 while Node 12 is still at 7.4 so I guess that’s the reason. So I guess the question is: Do we want to make this faster for the latest Chrome browsers or everything else, including all Node servers (since none of them are running V8 7.6 yet)? Node servers will also probably not be using the latest V8 for a while even if the latest version of Node has it.

[frederickfogerty]

Pulling in @jayeb to answer this from imgix's perspective. From my perspective, it's a tough call as there's no clear answer. I think we should accept this change as it is such a significant improvement, but aim to remove this in about a year's time (this could be put in a comment).

[jayeb]

I may be off-base here, but it seems to me that the performance of the spread operator in Chrome is mostly irrelevant--if this code is running in a browser, it's almost certainly going to be the Babel-compiled version, right? And since the compiled version isn't using the spread operator directly, it's kind of a moot point. This PR looks like an obvious win to me, but maybe I'm missing something.

[baldurh]

I may be off-base here, but it seems to me that the performance of the spread operator in Chrome is mostly irrelevant--if this code is running in a browser, it's almost certainly going to be the Babel-compiled version, right? And since the compiled version isn't using the spread operator directly, it's kind of a moot point. This PR looks like an obvious win to me, but maybe I'm missing something.

@jayeb Yes you’re absolutely right. I realised this also this morning but hadn’t gotten around to mention it 😄 So babel transpiles the spread operator to a function looping over the props of the object being spread. So that’s going to be slow in any environment 🙈

[sherwinski]

This gets a 👍 from me as well. Based on the conversation, it seems like Object.assign is the way we want to go here. Unless anyone has objections to wait, I'll merge today.