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

Add clarification about commands arguments #120

Closed
wants to merge 1 commit into from

Conversation

lpinca
Copy link
Contributor

@lpinca lpinca commented Aug 3, 2015

This tweaks a comment to make clear that when the number of arguments is big, using an array results in better performances.

Correct me if I'm wrong.

@luin
Copy link
Collaborator

luin commented Aug 3, 2015

Actually they should have similar performance since ioredis passes arguments to the flatten method of Lodash without checking whether they are arrays: https://github.com/luin/ioredis/blob/master/lib/command.js#L48

@lpinca
Copy link
Contributor Author

lpinca commented Aug 3, 2015

Yes, but using an array of arguments will "remove" this loop.

@luin
Copy link
Collaborator

luin commented Aug 3, 2015

Sounds interesting. I wrote a tests for it: https://gist.github.com/luin/d06770c736866e0571d0. The result shows the former form is little faster. It looks like _.flatten would be faster if the arguments are all plain strings/numbers: https://github.com/lodash/lodash/blob/3.10.0/lodash.src.js#L2122

@lpinca
Copy link
Contributor Author

lpinca commented Aug 3, 2015

That's indeed interesting, I would have never expected that.
Feel free to close this.

@luin luin closed this Aug 3, 2015
@lpinca lpinca deleted the add/clarification branch August 3, 2015 14:43
@lpinca
Copy link
Contributor Author

lpinca commented Aug 3, 2015

Here is my own test case which confirms my original assumption.
The difference is quite noticeable:

macbook:bench luigi$ node index.js 
without copy loop x 50,886 ops/sec ±0.64% (96 runs sampled)
with copy loop x 33,855 ops/sec ±0.38% (95 runs sampled)
Fastest is without copy loop

@luin
Copy link
Collaborator

luin commented Aug 3, 2015

I ran your test case and got the similar result as yours. I though there may be too few arguments in my previous test case, so I updated it but the results between the two forms are still slight. Don't know why.

@lpinca
Copy link
Contributor Author

lpinca commented Aug 3, 2015

I don't know either, but it doesn't matter, I doubt that someone will ever use a number of arguments big enough to make the difference.

@AVVS
Copy link
Collaborator

AVVS commented Aug 3, 2015

It could be that array wasnt optimized (diff types of values, not all strings for instance) or some other similar case

On 03 Aug 2015, at 09:56, Luigi Pinca notifications@github.com wrote:

I don't know either, but it doesn't matter, I doubt that someone will ever use an amount of arguments that will make a difference.


Reply to this email directly or view it on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants