loadMany 40X performance improvement, array observer notifications only on load completion. #800

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
7 participants
Contributor

alexspeller commented Mar 12, 2013

So, this started as an attempt to improve load performance, which it does, however it also solves another annoying bug, namely that observers fire for every single record loaded into a store.

The performance is way better in this version, for a large number of records (~10,000) load time is about ~500ms vs ~20s in chrome on a recent MacBook Pro. It also solves a really annoying problem of computed properties / observers, namely that when they observe an array that is loaded from ember data they fire for every object loaded into the store, not just when all the objects are loaded.

This problem can be demonstrated here in these jsFiddles.

Fiddle with ember-data master version

Fiddle with this version of ember-data

As you can see in the fiddles, my version only logs a property change when all models are loaded into the store, wheras for the current master there is a log for each individual object loaded into the store. When there are a large number of models this quickly becomes a problem (even for smaller numbers of models it is a pain, as if you have processing that takes e.g. 500ms in an array observer, and you then load 10 objects into that array, the app will freeze for 5 seconds).

There are failing tests in this pull request - I have not been able to fix them and they seem to run fine individually with rackup but not together. I don't have more time to work on fixing the tests at the moment and am using a different approach now, however these performance improvements and bugfix work great apart from the tests, so I'm filing this pull request in the hope that someone more experienced with the tests can take a look.

Owner

stefanpenner commented Mar 12, 2013

Seems like a good direction to take. I will take some time this evening to thoroughly review.

A high level concern: it does look like many new lines of code where added, but no test coverage.

Contributor

alexspeller commented Mar 12, 2013

Awesome.

As indicated in this comment, the new code is basically a refactoring of the code in the updateManyArrays and updateManyArray methods, and thus covered by existing tests. Perhaps a new test for the bug mentioned in the jsFiddle would make sense, but ultimately removing the updateManyArray and updateManyArrays methods entirely probably makes more sense.

I didn't want to do that because there are tests that break when you do that. These tests explicitly expect arrayContentDidChange to be called with arguments specifying which record was added, and this doesn't make sense any more if you're updating the array in a batch like this. However as there are tests explicitly about this behaviour I didn't want to remove it as it might cause breaking changes to users if they rely on it. This is why the old code path is still taken when only one record is changed, which is probably not what we want.

Contributor

alexspeller commented Mar 14, 2013

I cleaned this up a bit and made some more tests pass, I actually deleted the old method, there are still some tests failing but less now. If you would merge this if the tests were passing, let me know and I will keep bashing my head against the tests, but don't want to waste time if it's for nothing.

I'm generally against +1 posts, but this would help me out a lot as well.

Owner

wagenet commented Apr 16, 2013

@alexspeller Sorry for being so slow to comment on this. Ideally, we'd fix the behavior of arrays so this wouldn't even be an issue. That said, until we can do that, this might be a good stopgap. We'd also need to have all the tests passing of course :)

Any update to the status on this? I'm having big issues when trying to use ember data with endpoints that return 100 -> 5000 results. This sounds like it could be the reason. I'm willing to dive in and try making the tests pass if this is the direction the core team wants to go?

Contributor

alexspeller commented May 28, 2013

I'm afraid that I haven't had any more time to work on this - in fact I've pretty much stopped using ember data altogether. If you're working with endpoints returning 5000 results, I would recommend doing the same presently, I spent a long time fighting performance issues before giving up for large datasets :-(

Owner

stefanpenner commented May 28, 2013

@wagenet should we get this in as a short-term fix? (cc: @tomdale) ?

Owner

krisselden commented May 28, 2013

@stefanpenner tests fail, I think it was a WIP

Contributor

alexspeller commented May 28, 2013

There are failing tests, and after spending about 2 days bashing my head against them I gave up and went for a different approach. This needs someone who understands ember internals better than me to look at, I think. Sorry, I tried my best and failed :)

But is the idea sound? With some work and passing tests could it be merged in? I have an app that needs to load up to a few thousand records when initiated, and it kills the UI. That is if chrome doesn't flat out crash.

Contributor

alexspeller commented May 28, 2013

@raytiley I think the idea is sound, as far as I can tell - however, even with this performance enhancement, I think you will find ember-data too slow at the present time for multi-thousand recordsets. I was using this patched version in an app and even with this patch, using a custom solution was a lot faster.

Owner

stefanpenner commented May 28, 2013

@alexspeller alright, seems legit. We can treat this PR, as an identified problem, still pending proper solution.

Likely that perfect solution is making array observers themselves async.

Owner

wagenet commented Aug 10, 2013

I think this will be addressed by async observers and possibly computed array properties: emberjs/ember.js#2711

Owner

wycats commented Sep 4, 2013

@alexspeller can you rebase this against master and compare this with Ember Data master? I'm definitely still interested in this.

Contributor

alexspeller commented Sep 4, 2013

I will, I'm super busy this week but next week I'll post some up to date benchmarks

Owner

wagenet commented Oct 13, 2013

Contributor

alexspeller commented Oct 14, 2013

Sorry this took so long, unsurprisingly the code has changed massively and I had to basically start again.

In short, this is no longer relevant. A similar approach seems to have been taken to ensuring that record arrays are not updated inefficiently (see this function in RecordArrayManager).

So this approach to improving performance is obsolete and large performance gains have already been seen thanks to ongoing improvements. I am currently seeing times of 2-3 seconds after ajax response complete for 10,000 row recordset. Good work ED team!

@alexspeller alexspeller deleted the alexspeller:loadmany-performance branch Oct 14, 2013

Owner

stefanpenner commented Jul 31, 2014

@alexspeller if you combine ED master and Ember master you should see further improvements here. I have been putting a good amount of effort into this use-case and feel the ceiling is still extremely high.

if u want to augment https://github.com/stefanpenner/perf-stress to also include a route with something aligned to your specific scenario I will take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment