store.upsert(items) is very slow ($rootScope.$apply)
Opened this issue · 21 comments
ok found that $rootScope.$apply is being called.
notify: (args...) ->
$rootScope.$apply =>
@q.notify(args...)and let's take a quick look at upsert:
upsert: (data) ->
@_arrayOperation data, (item) =>
@store.put(item)turns out that all array operations are using notify.
_arrayOperation: (data, mapFunc) ->
defer = @defer()
data = [data] unless angular.isArray(data)
for item in data
req = mapFunc(item)
results = []
defer.rejectWith(req)
req.onsuccess = (e) ->
results.push(e.target.result)
defer.notify(e.target.result)
defer.resolve(results) if results.length >= data.length
if data.length == 0
return $q.when([])
defer.promise 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 $indexedDB, but it starts 1000 digest cycles...
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 notify or by the amount of watches on the page, making each notify expensive.
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 $rootScope.$apply calls. Why are they needed if $q-promises trigger the digest cycle anyway?
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?
Previous versions of angular did not do this.
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:
- The unit tests fail (timeout) after removing $apply
- These $apply calls sometimes cause the '$digest already in progress' exception. Don't know how to reproduce this. Mostly happens when I'm using debugger to debug some other part of the app.
Researched it a bit more. My apologies, $q promises don't trigger the digest. Quite the contrary, they indeed need some code to trigger the digest for them, otherwise their then callbacks won't fire. In this connexion, it's interesting to look at the implementation of $http. First, they use if (!$rootScope.$$phase) $rootScope.$apply(); instead of just $rootScope.$apply(); to avoid the exception I mentioned above. Second, there is an interesting option that enables using $applyAsync instead of $apply.
Yeah. I haven't looked into this a lot. My recollection when I rewrote it
of the angular documentation here is kind of spotty.
On Jun 11, 2015 4:22 PM, "thorn0" notifications@github.com wrote:
Researched it a bit more. My apologies, $q promises don't trigger the
digest. Quite the contrary, they indeed need some code to trigger the
digest for them, otherwise their then callbacks won't fire. In this
connexion, it's interesting to look
https://github.com/angular/angular.js/blob/master/src/ng/http.js at the
implementation of $http. First, they use if (!$rootScope.$$phase)
$rootScope.$apply(); instead of just $rootScope.$apply(); to avoid the
exception I mentioned above. Second, there is an interesting option that
enables using $applyAsync
https://docs.angularjs.org/api/ng/type/%24rootScope.Scope#%24applyAsync
instead of $apply.—
Reply to this email directly or view it on GitHub
#19 (comment)
.
$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.
$rootScope.$apply is used because the original author created his own implementation of a deferred/promise model called DbQ
No. It's needed for Angular promises. Their callbacks are called on digest. See my previous comment. It seems to me, using $applyAsync can help in your case.
You're right, $q promises themselves don't trigger a digest, but resolving a promise, such as with var deferred = $q.defer(), deferred.resolve(); triggers a digest. Calling resolve or reject calls the then and catch methods, respectively. Ben Nadel does a good study here.
Why are we using the $rootScope.$apply callbacks instead of just chaining then and catch methods directly to the $q.defer() object? I still don't get that...we have direct access to the onsuccess method of any transaction, why not simply invoke $q.defer() there?
I'll do a little digging, myself. I'd like to find the performance effect and whether it's necessary.
resolving a promise, such as with var deferred = $q.defer(), deferred.resolve(); triggers a digest
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 qFactory which takes a callback as its first parameter that, when fired, invokes $rootScope.$evalAsync. You can see in the definition for qFactory that this callback is called nextTick, which (to make a long story short) gets called when you call $q.defer().resolve or $q.defer().reject. Therefore, those two methods do invoke $rootScope.$evalAsync. And, as you can see below, $evalAsync then triggers a digest.
$provide.provider({
// ...
$q: $QProvider,
// ...
});
function $QProvider() {
this.$get = ['$rootScope', '$exceptionHandler', function($rootScope, $exceptionHandler) {
return qFactory(function(callback) { // this function(callback) is nextTick() later on
$rootScope.$evalAsync(callback); // <-- TRIGGERS DIGEST
}, $exceptionHandler);
}];
}
// ...
function qFactory(nextTick, exceptionHandler) {
// ...
function callOnce(self, resolveFn, rejectFn) {
// ...
function scheduleProcessQueue(state) {
// ...
nextTick(function() { processQueue(state); });
}
// ... inside the Deferred.prototype ...
resolve: function(val) {
// ...
this.$$resolve(val);
},
$$resolve: function(val) {
var then, fns;
fns = callOnce(this, this.$$resolve, this.$$reject);
// ...
if(isFunction(then) {
// ...
} else {
// ...
scheduleProcessQueue(this.promise.$$state)
// ...
Here's where the rubber meets the road...
$evalAsync: function(expr, locals) {
// ...
$rootScope.$digest(); // <-- o.O
So what?
A couple things.
Per the angular $q documentation:
$q is integrated with the $rootScope.Scope Scope model observation mechanism in angular, which means faster propagation of resolution or rejection into your models and avoiding unnecessary browser repaints, which would result in flickering UI.
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 $q is intended to be used. I say contradictory because the original author is wrapping $q in an object called DbQ which adds $rootScope.$apply callbacks everywhere.
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 $rootScope.$apply. Instead, if we cleared out all of the unnecessary $apply calls, and rely instead on the $evalAsync mechanism built directly into $q, then I believe we can alleviate this issue altogether.
I tried to remove all the $apply calls. It stopped working. Tests don't pass.
Yeah, there's probably more to it than just removing the $apply calls. It's the whole paradigm that the original author took and it's spread throughout the library (in the Transaction object as well). As far as the tests go, they'll probably need to be rewritten with the use of $q in mind; my guess is there are parameters of the test that are expecting the use of DbQ.
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 $q schedules callback execution on the next digest cycle, which executes asynchronously because $evalAsync doesn't call $digest directly. It uses a zero-second timeout to defer it. But an implicit call to $apply executes the digest cycle immediately so that the callbacks of promises are executed right away, synchronously. Looks like $http does the same for some reason.
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 $q to handle the digest, rather than invoking $apply directly would benefit us hugely.
Key takeaways from Ben's article:
The $evalAsync() queue, on the other hand, is flushed at the top of the while-loop that implements the "dirty check" inside the $digest. This means that any expression added to the $evalAsync() queue during a digest will be executed at a later point within the same digest.
To make this difference more concrete, it means that asynchronous expressions added by $evalAsync() from within a $watch() binding will execute in the same digest. Asynchronous expressions added by $applyAsync() from within a $watch() binding will execute at a later point in time (~10ms).
This makes it sound like when an indexedDB async method gets resolved (assuming it was resolved with the $q service), then any subsequent resolved method completed will get added to the same digest cycle that is running - rather than $apply, which invokes $digest directly.