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

Improve performance using array instead of linked-list to implement Backbone.Events #1284

Merged
merged 1 commit into from
May 5, 2012

Conversation

lifesinger
Copy link
Contributor

In Chrome 18:

the linked-list version: 97,270  57% slower
the array version: 227,369 fastest

http://jsperf.com/backbone-events-linked-list/8

Maybe it is worthy to merge this pull request.

@jashkenas
Copy link
Owner

It was originally changed from an array implementation to a linked list implementation for performance reasons. Mind taking a look at the original ticket, and letting me know what you think?

@lifesinger
Copy link
Contributor Author

@jashkenas Thanks for your information, I found this ticket:

#707

And the source code of the array implementation before the linked list version is:

https://github.com/braddunbar/backbone/blob/9982e314cbacb94429a838e480be20bb111cb73e/backbone.js

This array implementation has some "ugly" code to affect performance, such as:

  1. place slice.call in a loop
  2. use [[callback, context], [callback, context], ...] data structure
  3. leave null hole when unbind

In my array implementation, I have deal with carefully to fix all the performance issues.

A updated performance comparision:

http://jsperf.com/backbone-events-linked-list/8
http://jsperf.com/backbone-events-linked-list/9

It seems that the new array-implementation is very faster than the current linked-list version.

And more important, the array implementation is more readability than the linked-list version. I think it is worthy coming back to the array implementation with performance improving.

@braddunbar
Copy link
Collaborator

This is fantastic stuff @lifesinger! Sorry about the original request. I shouldn't have been so quick to close it. :)

I think the huge discrepancies in Chrome and Firefox are due to their copy-on-write array implementations. In the vast majority of cases the callback list won't be altered during trigger so this is a huge win. I'm going to merge this and make a couple of stylistic changes. Wonderful stuff, thanks @lifesinger!

braddunbar added a commit that referenced this pull request May 5, 2012
Improve performance using array instead of linked-list to implement Backbone.Events
@braddunbar braddunbar merged commit 7fcba24 into jashkenas:master May 5, 2012
@lifesinger
Copy link
Contributor Author

@braddunbar It is very happy to see this merge.

Except copy-on-write array implementations in chrome and firefox, another notable performance improving is:

changing

rest = slice.call(arguments, 1);

to

for (i = 1, length = arguments.length; i < length; i++) {
        rest[i - 1] = arguments[i];
      }

In most cases, the arguments.length is less than 5. So, use loop to set rest is more effective than slice.call. For readability, I suggest to add a comment before this loop.

Hope that Backbone is getting better and better.

@braddunbar
Copy link
Collaborator

Agreed. I was rather surprised that slice.call(arguments, 1) is so slow. It was accounting for almost half the time spent in trigger for small callback lists. Thanks for this @lifesinger!

jridgewell added a commit to jridgewell/backbone that referenced this pull request Feb 5, 2015
[js perf](http://jsperf.com/backbone-events-linked-list/18)

Roughly equivalent performance. Using a linked list allows the events to
be mutated by trigger, without skipping an event. This is a **breaking
change**.

Events used to use a linked list (and switched to an array in jashkenas#1284).
But, it performed [very badly](http://jsperf.com/backbone-events-linked-list/8).
I'm fairly certain that was due to
[L106](https://github.com/jashkenas/backbone/blob/6c3d3838e10d5a070f0d1f604fb5c99ed34c8c93/backbone.js#L106)
recreating the linked list's object every `#on`, and
[L138](https://github.com/jashkenas/backbone/blob/6c3d3838e10d5a070f0d1f604fb5c99ed34c8c93/backbone.js#L138)
calling `#on` to re-bind callbacks.

This fixes jashkenas#3466 and closes jashkenas#3463.
jridgewell added a commit to jridgewell/backbone that referenced this pull request Feb 5, 2015
[js perf](http://jsperf.com/backbone-events-linked-list/18)

Roughly equivalent performance. Using a linked list allows the events to
be mutated by trigger, without skipping an event. This is a **breaking
change**.

Events used to use a linked list (and switched to an array in jashkenas#1284).
But, it performed [very badly](http://jsperf.com/backbone-events-linked-list/8).
I'm fairly certain that was due to
[L106](https://github.com/jashkenas/backbone/blob/6c3d3838e10d5a070f0d1f604fb5c99ed34c8c93/backbone.js#L106)
recreating the linked list's object every `#on`, and
[L138](https://github.com/jashkenas/backbone/blob/6c3d3838e10d5a070f0d1f604fb5c99ed34c8c93/backbone.js#L138)
calling `#on` to re-bind callbacks inside `#off`.

This fixes jashkenas#3466 and closes jashkenas#3463.
jridgewell added a commit to jridgewell/backbone that referenced this pull request Feb 7, 2015
[js perf](http://jsperf.com/backbone-events-linked-list/18)

Roughly equivalent performance. Using a linked list allows the events to
be mutated by trigger, without skipping an event. This is a **breaking
change**.

Events used to use a linked list (and switched to an array in jashkenas#1284).
But, it performed [very badly](http://jsperf.com/backbone-events-linked-list/8).
I'm fairly certain that was due to
[L106](https://github.com/jashkenas/backbone/blob/6c3d3838e10d5a070f0d1f604fb5c99ed34c8c93/backbone.js#L106)
recreating the linked list's object every `#on`, and
[L138](https://github.com/jashkenas/backbone/blob/6c3d3838e10d5a070f0d1f604fb5c99ed34c8c93/backbone.js#L138)
calling `#on` to re-bind callbacks inside `#off`.

This fixes jashkenas#3466 and closes jashkenas#3463.
jridgewell added a commit to jridgewell/backbone that referenced this pull request Feb 7, 2015
[js perf](http://jsperf.com/backbone-events-linked-list/18)

Roughly equivalent performance. Using a linked list allows the events to
be mutated by trigger, without skipping an event. This is a **breaking
change**.

Events used to use a linked list (and switched to an array in jashkenas#1284).
But, it performed [very badly](http://jsperf.com/backbone-events-linked-list/8).
I'm fairly certain that was due to
[L106](https://github.com/jashkenas/backbone/blob/6c3d3838e10d5a070f0d1f604fb5c99ed34c8c93/backbone.js#L106)
recreating the linked list's object every `#on`, and
[L138](https://github.com/jashkenas/backbone/blob/6c3d3838e10d5a070f0d1f604fb5c99ed34c8c93/backbone.js#L138)
calling `#on` to re-bind callbacks inside `#off`.

This fixes jashkenas#3466 and closes jashkenas#3463.
jridgewell added a commit to jridgewell/backbone that referenced this pull request Mar 5, 2015
[js perf](http://jsperf.com/backbone-events-linked-list/18)

Roughly equivalent performance. Using a linked list allows the events to
be mutated by trigger, without skipping an event. This is a **breaking
change**.

Events used to use a linked list (and switched to an array in jashkenas#1284).
But, it performed [very badly](http://jsperf.com/backbone-events-linked-list/8).
I'm fairly certain that was due to
[L106](https://github.com/jashkenas/backbone/blob/6c3d3838e10d5a070f0d1f604fb5c99ed34c8c93/backbone.js#L106)
recreating the linked list's object every `#on`, and
[L138](https://github.com/jashkenas/backbone/blob/6c3d3838e10d5a070f0d1f604fb5c99ed34c8c93/backbone.js#L138)
calling `#on` to re-bind callbacks inside `#off`.

This fixes jashkenas#3466 and closes jashkenas#3463.
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.

4 participants