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

Example of severe speed regression in ember-data #20

Merged
merged 3 commits into from
Nov 9, 2014

Conversation

vitch
Copy link

@vitch vitch commented Nov 9, 2014

As discussed on the forum, when I upgraded my app from Ember 1.7 and Ember Data beta 9 to Ember 1.8 and Ember Data beta 11 I noticed a large slowdown in the time taken to push records into the store.

This PR contains code which does what my app does. Apologies for the large number of models but I thought it was best to illustrate the relationships etc I have set up.

Running this code with Ember 1.7 and Ember Data beta 9 takes a total of around 1.6 seconds on my laptop. Running it with Ember 1.8 and Ember Data beta 11 takes about 2.6 seconds - a 62% slowdown!

On my target platform of Android the code runs much slower and so the slow down is even more significant.

Hopefully this example code can help tracking down where things have slowed down!

vitch added 3 commits November 9, 2014 23:01
The models and data are indicative of what I'm using in my app. I noticed
a significant slowdown on Android (my target platform) for adding data to
the store when I upgraded from Ember 1.7 and Ember Data beta 9 to
Ember 1.8 and Ember Data beta 11 - this should allow other people to
reproduce the slowdown...
@stefanpenner
Copy link
Owner

thanks.

cc @igorT

@vitch
Copy link
Author

vitch commented Nov 9, 2014

(navigate to http://localhost:4200/push-payload after checking out this branch to see how long it takes on your setup)

stefanpenner added a commit that referenced this pull request Nov 9, 2014
Example of severe speed regression in ember-data
@stefanpenner stefanpenner merged commit 840fead into stefanpenner:master Nov 9, 2014
@stefanpenner
Copy link
Owner

can you explain the point of the run.next's?

export default DS.Model.extend({
requiredType: DS.belongsTo('equipment_type'),
chosenEquipment: DS.belongsTo('equipment_item'),
equipment: [],
Copy link
Owner

Choose a reason for hiding this comment

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

this seems like a mistake, are you sure you want the relationship on the prototype?

Copy link
Author

Choose a reason for hiding this comment

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

You're right, that's a mistake / some old code. Removing the line the app still works (when I create requiredItems I set equipment: homeController.get('equipmentItems' + requiredType.get('id')),) - I guess I could define the property as DS.hasMany('equipment_item') although this data is never sent over the wire (I add it to the model so that when the model is passed into a component the component has access to the equipment items that can be selected from)

@stefanpenner
Copy link
Owner

most of the time is being spent handling the 3 relationships of equipmentItems

@stefanpenner
Copy link
Owner

  • equipment_type
  • package_level
  • rented_item

@stefanpenner
Copy link
Owner

So interestingly this example does not eagerly consume the relationships but yet they impose a considerable performance impact on pushPayload. A quick, (and scenario solving hack) shows that if the relationships were derived on-demand the above mentioned scenario would be 4-5 times faster.

c0ee8fb

@igorT I'm curious if it would be possible to lazily construct relationships in ED, if so what would be the steps to accomplish this? If not, I'm curious as to the reason.

@stefanpenner
Copy link
Owner

emberjs/data#2289 would also help out alot

@vitch
Copy link
Author

vitch commented Nov 10, 2014

Thanks for looking into this!

can you explain the point of the run.next's?

In the actual app on the target platform the app was freezing for 20-30 seconds on startup while it did the inserts. The run.nexts gave it an occasional frame to update a bootstrap animating progress bar so it didn't seem quite so broken.

Thanks so much for the hack / workaround - I'm trying to apply that to the actual code base now and will report back on how it works out. On this repository the hack seems to have removed the difference between ember-data.beta9 and beta11 which bodes well for my app...

@vitch
Copy link
Author

vitch commented Nov 10, 2014

Thanks again for the workaround. I can confirm that it gave my real world application a massive performance boost. I had to make a couple of tweaks in places where I accessed equipmentItem.type.name and called toLowerCase on it because it now initially comes back as undefined but other than that everything works great.

There were a couple of slight tweaks I made in 6b2831e once I'd put the code back into my app but other than that it's great!

Thanks again - it would be great if in the next version of ember-data you don't need this workaround but for now this has helped my app immeasurably 😄

@vitch vitch deleted the feature/push-payload branch November 23, 2014 14:00
@vitch
Copy link
Author

vitch commented Nov 23, 2014

Sorry to resurrect an old issue but I've noticed that the suggested workaround doesn't work transparently with my app... In fact there are some unexpected bugs that crop up where I'm consuming the properties...

e.g. In a computed property on a component somewhere I have code like:

if (this.get('chosenEquipment.type.hasPackage')) {

Where chosenEquipment is an equipment-item. Before applying the workaround the type property on an equipment-item was a belongsTo relationship and the line above worked fine. After applying the workaround the type property is initially a Promise (as returned from the call to store.find) so hasPackage is undefined.

Maybe I do need the eager upfront loading that was slowing down initialisation? Or can you see a workaround when consuming the properties? I guess I could return a Promise from the computed property and wait for any dependencies then calculate my value but I'm wondering if there is something less manual? Or if there is a chance that ED will lazily construct relationships in future so I wouldn't need the workaround?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants