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

Backport empty-object performance improvements from 2.1.0-beta1 #3689

Merged
merged 1 commit into from
Sep 2, 2015

Conversation

jnfisher
Copy link

We found these changes substantially improved loading of large datasets in our app.

This PR contains code from the following 2.1.0-beta1 PRs:
#3641 Improve InternalModel (2x - 3x faster)
#3649 Empty object

emberjs#3641 Improve InternalModel (2x - 3x faster)
emberjs#3649 Empty object
@stefanpenner
Copy link
Member

We found these changes substantially improved loading of large datasets in our app.

I would be curious to know the #s you have seen in your app with your data.

@jnfisher
Copy link
Author

In our app we have a large, hierarchical tree of data that is pulled and deserialized when users visit the site.

To test, we monkey patched these two PRs into the latest 1.13 build of Ember-Data and found loading times improved by 50% (for a dataset containing on the order of hundreds of complex DS.Models with relationships the time to load went from 40s to 20s, roughly). We still need to make it faster but this was a nice bump for a simple change!

@stefanpenner
Copy link
Member

@jnfisher good to hear, those numbers still aren't very but our headroom for improvement here is still very high.

It is also worth noting, those two optimizations had a third counter part, one that made OrderSet and Set insertion much faster.
emberjs/ember.js#12061

Which was back-ported to 1.13-stable (but not yet part of a 1.13 release).

I am curious if you are also utilizing that.

@stefanpenner
Copy link
Member

@jnfisher when i took a look at this, i also noticed InternalModel tends to allocate many members upfront, event thought not all of them are always needed.

Related issue (I also added some ideas on how to implement) #3644

@jnfisher
Copy link
Author

@stefanpenner I don't recall if we were using stable or release in our testing (we're still on a branch until we clear out some remaining problems with stained-by-children and fragments)-- but we'll keep an eye on it.

Agree that 10s+ load times are not great. Fortunately, our users accept waiting for an upfront loading time if the app is fast afterwards. It's a work in progress.

@rwjblue
Copy link
Member

rwjblue commented Aug 22, 2015

emberjs/ember.js#12061 which was back-ported to 1.13-stable (but not yet part of a 1.13 release)

It was included in Ember 1.13.8.

@stefanpenner
Copy link
Member

@bmac @igorT @fivetanley @wecc r?

igorT added a commit that referenced this pull request Sep 2, 2015
Backport empty-object performance improvements from 2.1.0-beta1
@igorT igorT merged commit b08fe53 into emberjs:v1.13 Sep 2, 2015
@igorT
Copy link
Member

igorT commented Sep 2, 2015

I'm a bit concerned we leak NullObjects to user space. People might be surprised by that? @stefanpenner any thoughts? Should be simple to wrap changedAttributes to avoid that, and the perf impact would be minimal

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