Giter Club home page Giter Club logo

Comments (21)

dnozay avatar dnozay commented on July 4, 2024
            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.

from angular-indexeddb.

bramski avatar bramski commented on July 4, 2024

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.

from angular-indexeddb.

bramski avatar bramski commented on July 4, 2024

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.

from angular-indexeddb.

dnozay avatar dnozay commented on July 4, 2024

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.

from angular-indexeddb.

bramski avatar bramski commented on July 4, 2024

Can you close your issue? Is this a problem with this library or with your implementation?

from angular-indexeddb.

thorn0 avatar thorn0 commented on July 4, 2024

@bramski Throughout the library, you have a lot of $rootScope.$apply calls. Why are they needed if $q-promises trigger the digest cycle anyway?

from angular-indexeddb.

bramski avatar bramski commented on July 4, 2024

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?

from angular-indexeddb.

thorn0 avatar thorn0 commented on July 4, 2024

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

from angular-indexeddb.

bramski avatar bramski commented on July 4, 2024

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.

from angular-indexeddb.

thorn0 avatar thorn0 commented on July 4, 2024

Don't have this information as well, but:

  1. The unit tests fail (timeout) after removing $apply
  2. 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.

from angular-indexeddb.

thorn0 avatar thorn0 commented on July 4, 2024

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.

from angular-indexeddb.

bramski avatar bramski commented on July 4, 2024

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" [email protected] 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)
.

from angular-indexeddb.

theBull avatar theBull commented on July 4, 2024

$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.

from angular-indexeddb.

thorn0 avatar thorn0 commented on July 4, 2024

$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.

from angular-indexeddb.

theBull avatar theBull commented on July 4, 2024

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.

from angular-indexeddb.

thorn0 avatar thorn0 commented on July 4, 2024

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.

from angular-indexeddb.

theBull avatar theBull commented on July 4, 2024

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.

from angular-indexeddb.

thorn0 avatar thorn0 commented on July 4, 2024

I tried to remove all the $apply calls. It stopped working. Tests don't pass.

from angular-indexeddb.

theBull avatar theBull commented on July 4, 2024

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.

from angular-indexeddb.

thorn0 avatar thorn0 commented on July 4, 2024

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.

from angular-indexeddb.

theBull avatar theBull commented on July 4, 2024

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.

from angular-indexeddb.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.