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

Use for-loop, instead of slice #3489

Merged
merged 1 commit into from
Feb 23, 2015

Conversation

jridgewell
Copy link
Collaborator

http://jsperf.com/trigger-with-for-loop-vs-slice

One of the optimizations I've found in the linked-list events branch is using a for-loop to grab the trigger arguments. Perf testing #trigger on that branch (with slice) against master doesn't tell the whole truth, since this slice call is the performance bottleneck. My linked-list trigger should be roughly ~8% slower than a simple array, but it's a dead heat as long as slice is in there.

@jashkenas
Copy link
Owner

Hrm. I don't know if 8% is worth it for that gross code ;)

@jamiebuilds
Copy link

@jashkenas It would definitely be worth it to me as trigger is the hottest method in all of Backbone. When I measured it a few months back it was called around 5000x on application startup in my company's app. Slicing the arguments object keeps it from optimizing that code path as much as it could.

@jridgewell
Copy link
Collaborator Author

Oh no, this is a 2000% (not a typo!) boost. I'm saying my linked-list code should be 8% slower, but it's the exact same speed because this slice is bottle-necking #trigger.

@jashkenas
Copy link
Owner

2000% boost? Now I'm confused. But holy hell.

Would this be worth pulling out as a little fastSlice helper function, and using anywhere else we're slicing an incoming arguments array?

@jashkenas
Copy link
Owner

Also, that JSPerf looks questionable. Why would three arguments be so much faster than no arguments?

Let's be careful here, before mucking things about.

@jridgewell
Copy link
Collaborator Author

Would this be worth pulling out as a little fastSlice helper function, and using anywhere else we're slicing an incoming arguments array?

That would help, but it would still de-opt because of leaking arguments. We could just use _.toArray most of the time.

Also, that JSPerf looks questionable. Why would three arguments be so much faster than no arguments?

I'm just as confused as you are. I think it's because the method isn't getting jit-d since there's not a hot loop.

@jridgewell
Copy link
Collaborator Author

Gah, one of these days I'm going to spot the errors before I post them in a PR. Here's an corrected jsperf, showing 500% increases. (I was calling For.Events.trigger, not For.trigger)

@megawac
Copy link
Collaborator

megawac commented Feb 11, 2015

Ping jashkenas/underscore#1758. Mind testing with a _.slice utility that a) has the extra step arg b) doesn't (ttvm :D)

@jridgewell
Copy link
Collaborator Author

Mind testing with a _.slice utility that a) has the extra step arg b) doesn't (ttvm :D)

I'll do that shortly.

@jashkenas
Copy link
Owner

One would think that would hit your "leaking arguments" problem, but it's worth a shot.

@jridgewell
Copy link
Collaborator Author

@megawac: http://jsperf.com/trigger-with-for-loop-vs-slice/3

Using _.slice is (roughly 200%) faster than slice.call in both circumstances, but still slower than inline for-loop due to the leaky arguments. Also, it seems like it's trying to do too much: http://jsperf.com/cr-52768/6

@jameshartig
Copy link
Contributor

First, thanks @jridgewell for finding this perf boost! While I agree that slice is more elegant the one-line for loop seems worth it in this case, since it's in such a hot path.

@megawac
Copy link
Collaborator

megawac commented Feb 13, 2015

Would it be more appropriate to optimize _.rest to not use slice.call or any argument juggling @jridgewell?

Talk to me on gitter, ill update a jsperf tonight

@jameshartig
Copy link
Contributor

But I think he's saying that it'll deopt if we pass arguments anywhere. So even passing _.rest would induce a deopt and cause it to be slower.

@megawac
Copy link
Collaborator

megawac commented Feb 14, 2015

A solid rest implementation will give this a run for its money

http://jsperf.com/trigger-with-for-loop-vs-slice/5
https://github.com/megawac/backbone/compare/rest-test

megawac added a commit to megawac/underscore that referenced this pull request Feb 14, 2015
@jdalton
Copy link
Contributor

jdalton commented Feb 14, 2015

That jsperf was super convoluted, I cleaned it up. @jridgewell is right, passing arguments to a function will force engines to actually create the arguments object so while you may be using a faster slice-like implementation (on small to medium array-likes, reasonable for arguments) you'd still miss out on the opt of not having to create a real arg object. Engines like Chakra will materialize the arguments object in the presence of formal params too (It's something we'll fix) so that's something to keep in mind. In the revised benchmark _.rest is the slowest.

In most cases I convert the arguments object to an array in an external exposed method then pass it off as an array to internal helpers (similar to what Backbone is doing). Applying this practice to other things that are known to disqualify optimizations lets the smaller external API's tackle the disqualifications but leaves internal functions available for optimizations. In special cases I'll use a for-loop (like with a partial implementation) but that's not a common case (the gist, don't obsess all the time over it).

jameshartig pushed a commit to mc0/bedrock that referenced this pull request Feb 16, 2015
@megawac
Copy link
Collaborator

megawac commented Feb 17, 2015

👍

jashkenas added a commit that referenced this pull request Feb 23, 2015
@jashkenas jashkenas merged commit caafa67 into jashkenas:master Feb 23, 2015
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

6 participants