Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stringify: Avoid arr = arr.concat(...), push to the existing instance #269

Merged

Conversation

papandreou
Copy link
Contributor

@papandreou papandreou commented Jul 26, 2018

Currently qs.stringify will create a new array whenever a new parameter is added. This fixes it by pushing to the existing one instead.

Very unscientific benchmark:

time node -e "const qs = require('./');for (let i = 0 ; i < 1000000 ; i += 1)qs.stringify({a: 1, b: 2, c: 3, d: 4, e: 5, f: 6, g: 7, h: 8, i: 9, j: 10, k: 11, l: 12, m: 13, n: 14, o: 15, p: 16, q: 17, r: 18, s: 19, t: 20, u: 21, v: 22, x: 23, y: 24, z: 25});"

Before:

real	0m11.247s
user	0m11.260s
sys	0m0.093s

After:

real	0m6.291s
user	0m6.305s
sys	0m0.052s

Copy link
Owner

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hesitant to do this; 5 seconds for a million iterations means that in the common case, it will be a scattering of milliseconds' difference.

lib/stringify.js Outdated
@@ -15,6 +15,14 @@ var arrayPrefixGenerators = {
}
};

var pushToArray = function (arr, valueOrArray) {
if (Array.isArray(valueOrArray)) {
Array.prototype.push.apply(arr, valueOrArray);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this changes a dependency on concat to a dependency on both push and apply.

Separately, concat actually has different semantics - it uses isConcatSpreadable, which includes more than just arrays (altho perhaps that doesn't matter here).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it bad to depend on Array#push and apply? They're fairly standard :). It'd look nicer with array spread, of course.

I had no idea about Symbol.isConcatSpreadable. As far as I can tell, the operand will always be an array or a string, though.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're all standard, but the more things that the package depends on existing, the more likely it will break if people mess with the builtin prototypes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's technically correct, but is it really worth worrying about? It seems unlikely that anyone will overwrite ES3 methods from prototypes in this day and age? Especially methods that have been in the language for 20 years?

I'm caught a bit off-guard because I haven't heard that kind of reasoning for preferring one piece of code over another for a very long time. I'm aware of MooTools, should.js and some of the even crazier things people did before, but hasn't the community stopped doing it and moved on at this point?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the web; I always write all my code assuming that everything will be deleted off the prototype (or replaced with a malicious version) after my code runs :-)

This code predates me, and your PR is changing the attack surface. I agree it's a minor concern in practice, but it's still nonzero.

@papandreou
Copy link
Contributor Author

Well, okay, I'm not totally invested in this. Just seemed wasteful and O(n^2)-y to create all those intermediate arrays.

@papandreou papandreou force-pushed the feature/avoidIntermediateArraysInStringify branch from 4df4608 to 829f866 Compare September 17, 2018 20:39
@papandreou
Copy link
Contributor Author

@ljharb, I've rebased this branch now. Still a bit baffled about your initial reaction to this change, do you still feel the same way?

Copy link
Owner

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made some small tweaks so that now the only runtime dependency is "apply"; I guess this is fine to merge.

@ljharb ljharb merged commit b853cb8 into ljharb:master Nov 23, 2018
ljharb pushed a commit that referenced this pull request Jan 11, 2022
ljharb pushed a commit that referenced this pull request Jan 11, 2022
ljharb pushed a commit that referenced this pull request Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants