When adding relations, use a reverse index and one event handler #454

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants

corps commented Apr 16, 2014

Attempts to reduce algorithmic complexity of looking up potential relations added to a model's master collection by requiring only one handler The code is not ideal in style, and could use some additional test exercising, but demonstrates the idea and has been used in my production app successfully.

Collaborator

philfreo commented Apr 29, 2014

Big +1 for working on performance in Backbone-Relational...

So that this can get further tested & merged... could you do a cleanup on the code style, remove usage of global window object, etc?

Collaborator

philfreo commented Apr 29, 2014

@PaulUithol any thoughts on the overall concept?

Owner

PaulUithol commented May 7, 2014

@corps, thanks for sharing your work so far!

I like the concept a lot, obviously; we could use a speed boost, and this looks like a pretty sane upgrade. I'm pretty strapped for time though; I will need time to read up a bit on what's happening here and to review/improve the current state of this solution.

Owner

PaulUithol commented May 25, 2014

I'm pondering what the best place is to let this implementation live; the store seems better suited. Also, looks like it should be possible to also handle removal via a reverse index, rather than separate relational:remove events per relation?

PaulUithol added a commit that referenced this pull request May 30, 2014

Owner

PaulUithol commented May 30, 2014

Okay, I've taken most of the changes in this PR together with some others in the performance branch (https://github.com/PaulUithol/Backbone-relational/tree/performance). Please try that one if you're interested. I've also converted remove to a direct list of relations.

To be honest, I don't see that much improvement yet in the few performance related tests we have; but those are highly artificial (we could use better ones). It also seems that manipulating lots of small objects (as the reverseIndex does) can be quite slow in some JS implementations; maybe we could improve that still.

gorman commented May 31, 2014

In my not-so-scientific tests, I see a consistent 30% performance improvement with this branch when loading a rather large/complex data set.

I experienced one crasher though: calling Backbone.Relational.store.unregister on a collection results in "Uncaught TypeError: Cannot read property 'removeRelated' of undefined" on https://github.com/PaulUithol/Backbone-relational/blob/performance/backbone-relational.js#L668

Owner

PaulUithol commented Jun 3, 2014

Thanks, sounds good. I'll look into that crasher.

@bpatram bpatram added this to the v0.11.0 milestone Mar 28, 2016

@bpatram bpatram added the enhancement label Mar 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment