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

Refs perf enhancement #238

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

cjblomqvist
Copy link

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
Copy link
Author

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
Copy link
Author

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

@cjblomqvist
Copy link
Author

@distracteddev
Copy link
Member

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

@cjblomqvist
Copy link
Author

Thanks!

@cjblomqvist
Copy link
Author

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
Copy link
Contributor

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

for(var i = 0, len = keys.length; i < len; i++) {
if(keys[i] === '$items') continue;

arr.concat(flatten(obj[i]));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little bug leading to crashes: the line should read

arr.concat(flatten(obj[keys[i]]));

@cjblomqvist
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

Good to know, thanks for sharing!

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.

3 participants