-
Notifications
You must be signed in to change notification settings - Fork 49
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
store.upsert(items) is very slow ($rootScope.$apply) #19
Comments
defer.notify(e.target.result) this piece is making my UI very very slow. I am retrieving 1000 items from a REST call, processing them and inserting them in It would be great if we had a way to opt out from the notify calls. |
I'm not sure if anybody cares too much about this. I don't see a big deal with adding configuration to opt-out and then making a major version upgrade so people don't get surprised. Sucks that angular $q has a perf issue here when nobody is listening for the updates and still issues the $digest. |
Suggest you open a PR with a fix that will work for you and we can get it released fairly quickly. I don't have time to do this personally. |
It may be a different problem altogether. http://stackoverflow.com/questions/18499909/how-to-count-total-number-of-watches-on-a-page - there are 50k+ watches on the page; so I am not sure anymore if the problem is caused by |
Can you close your issue? Is this a problem with this library or with your implementation? |
@bramski Throughout the library, you have a lot of |
Previous versions of angular did not do this. It may be able to be removed at this point. I'm unsure. Is someone who is seeing a perf issue going to benchmark and see? |
They always did AFAIK. That's one of the main reasons for Angular to have its own implementation of promises. See angular/angular.js#6697 |
Sure. The $rootScope.$apply was leftover from the original implementer. I had no reason to change it. If someone can show (a) that it's unnecessary (b) that it's detrimental to performance then sure let's remove it. I have no quantitative information right now. |
Don't have this information as well, but:
|
Researched it a bit more. My apologies, |
Yeah. I haven't looked into this a lot. My recollection when I rewrote it
|
$rootScope.$apply is used because the original author created his own implementation of a deferred/promise model called DbQ. It acts as a wrapper around Angular's $q service. I was curious about this in Issue # 39: Is the DbQ object really necessary? Why not just use $q I tinkered with this a little by replacing the DbQ deferred promise in a couple places with the $q.defer() object, but I believe we'd have to revamp the entire architecture of deferred promises within the library in order to effectively handle this. In any case, that would enable us to get rid of the $rootScope.$apply calls. As far as doing a bulk notify call after a series of upserts, I am totally onboard with that. I have exactly the same scenario - loading 1,000 items over a REST request. Because these upserts are individually bound to the $apply call, and because Angular renders bindings in the UI when a $digest occurs (is that correct?), the high number of digests are causing a rendering freeze. |
No. It's needed for Angular promises. Their callbacks are called on digest. See my previous comment. It seems to me, using |
You're right, Why are we using the I'll do a little digging, myself. I'd like to find the performance effect and whether it's necessary. |
That's not true. Just read the source of $q if you don't believe. |
I did. And it does. Notice how the $QProvider returns a function called
Here's where the rubber meets the road...
So what?A couple things. Per the angular
That whole part about "faster propagation of resolution or rejection into your models" is what all of that code above is doing. Resolving or rejecting your async promise is going to propagate up to your model. Now, because I am seeing a huge number of unnecessary browser repaints when I make a series of upserts, that tells me the library is doing something directly contradictory to the way Writing to a database, regardless of whether it's local, should not affect the UI. That's the whole reason indexedDB is async, so that it is non-blocking. That means the deferred/promise mechanism in the library is faulty, and the chief culprit is the unnecessary use of directly invoking |
I tried to remove all the |
Yeah, there's probably more to it than just removing the It'll be some hefty rework to get all this in place, but that's the difference between making this library decent and making it kick ass :]. As soon as I get time I'll jump on it. |
Looked into it again, you're right. So |
Yeah. I highly recommend you check out this article on $applyAsync vs $evalAsync...Ben Nadel again...Let me know what you think. I think the combination of relying on Key takeaways from Ben's article:
This makes it sound like when an indexedDB async method gets resolved (assuming it was resolved with the |
ok found that
$rootScope.$apply
is being called.and let's take a quick look at
upsert
:turns out that all array operations are using
notify
.The text was updated successfully, but these errors were encountered: