Backbone-relational icon indicating copy to clipboard operation
Backbone-relational copied to clipboard

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

Open corps opened this issue 11 years ago • 7 comments

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.

corps avatar Apr 16 '14 07:04 corps

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?

philfreo avatar Apr 29 '14 17:04 philfreo

@PaulUithol any thoughts on the overall concept?

philfreo avatar Apr 29 '14 17:04 philfreo

@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.

PaulUithol avatar May 07 '14 15:05 PaulUithol

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 avatar May 25 '14 15:05 PaulUithol

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.

PaulUithol avatar May 30 '14 11:05 PaulUithol

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

gorman avatar May 31 '14 07:05 gorman

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

PaulUithol avatar Jun 03 '14 21:06 PaulUithol