search

derbyjs / racer

Realtime model synchronization engine for Node.js
1.19k stars 117 forks source link

Refs perf enhancement #238

Open cjblomqvist opened 8 years ago

cjblomqvist commented 8 years ago

This PR greatly increases performance for refs, especially when you have many refs and you encounter many "index" (array) -events simultaneously. This is due to the fact that all refs are looped through and checked for each event. A simple scenario with 100 refs and 100 simultaneous events causes 10k loops.

Basically, instead of looping through all refs for each event, I've created a smarter structure to maintain maps. Instead of just a regular simple map with one level as depth, my smarter map structure maintains each level similar to the model in general. This makes for very efficient lookups when you'd otherwise have to loop through all items and check with utils.contains. Basically, you can just grab all relevant items for a given set of segments (path) in a hash-way instead.

Note that the PR does not alter the way stuff works, it just utilizes a more efficient data structure to speed things up.

Why do we need this kind of performance for refs? Well, you will easily have a lot of events in an app. If you also have a lot of refs, you'll easily stumble upon this performance issue. More specifically, in one of our apps on one page where we have a significant amount of refs (hundreds, almost a thousand), we had significant performance issues. Basically, we had a lot of refs due to using https://github.com/BBWeb/derby-view - which easily creates a lot of refs for you (but for good reason).

I'd greatly appreciate someone reviewing and accepting this PR.

cjblomqvist commented 8 years ago

By the way, if this gets pulled the Path structures could easily be separated out from refs and utilized wherever else FromMaps are used (e.g. filters, fns) to gain similar performance gains.

cjblomqvist commented 8 years ago

Oh, and btw. the performance gains in our app was at least 100x for this specific scenario...

cjblomqvist commented 8 years ago

@nateps @enjalot @distracteddev

distracteddev commented 8 years ago

@cjblomqvist Will have a look over this with Nate soon. Sorry for the delay!

cjblomqvist commented 8 years ago

Thanks!

cjblomqvist commented 8 years ago

I don't know what your definition of soon is, but it's been a while now. I also see your PR for the speedier bundling/reloading hasn't either been merged in. Sad to say, but I guess everything is as normal ;)

michael-brade commented 8 years ago

Wow! My derby-entity and derby-select2 components are heavily based on refs, and subjectively, this PR is a HUGE improvement!

cjblomqvist commented 8 years ago

Happy to hear my open source efforts are helping someone! :-)

Unfortunately I probably won't have the time to properly review and test your fix for the little bug you found! (but glad you posted here in case I run into it)

michael-brade commented 8 years ago

Oh yeah, thanks for posting it here! That work you have done certainly wasn't finished in an hour :) And the speed with 100+ refs became noticably worse without this, so it's very valuable.

However, my fix really is obvious and shouldn't take you much time: you first get the keys with Object.keys(obj);, then iterate over keys[], and then call obj[i] without using the keys array. A simple oversight. You did it correctly in flattenObj().

michael-brade commented 8 years ago

Some more information: while hunting a bug down (which turns out to be a known but rather difficult to fix bug in racer) I found that this change breaks the updateIndices ref-option: the respective unit tests all fail. And also, the crash I referenced above is triggered by the updateIndices option without my fix.

cjblomqvist commented 8 years ago

Good to know, thanks for sharing!