Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

feat(*): coalesce $apply calls which occur close together WIP #8736

Closed
wants to merge 2 commits into from

Conversation

caitp
Copy link
Contributor

@caitp caitp commented Aug 22, 2014

This is a work-in-progress, implementing what has been discussed with @kseamon and @IgorMinar

There are some open questions about this still, this first commit is a bit more feature-packed
than the original implementation, and maybe some of that stuff should be trimmed down or changed
a bit.

Adding support for $http now.

}));


it('should allow use of locals', inject(function($rootScope, $browser) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is something that we might not need, since we might not want to keep an object alive until the next turn.

But it's consistent with $eval, so maybe it's not bad

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a valid concern that locals would change between the call and the execution (Should we make a copy)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's worth caring if locals change, users could make a copy themselves if they know it's going to change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or alternatively we could just not support locals at all (if it's a private api and we don't use locals for it anywhere, there's no real reason to support locals unless we want to use it as an alternative to $applyAsync for certain event directives or something)

@caitp caitp added this to the 1.3.0 milestone Aug 22, 2014
@caitp
Copy link
Contributor Author

caitp commented Aug 22, 2014

@IgorMinar PTAL --- I've omitted the "configurable coalesced apply wait time" because I couldn't really tell if you were saying use it or don't use it in the message on hangouts --- plus it didn't look like there was a real benefit to using it in testing.

So, one of the things that kind of throws a rock in this is the way we send the request as the last interceptor, so we get a full $digest anyways because of $evalAsync. This means each request is going to have a minimum of one digest all on its own, regardless of coalesced apply.

... I guess it could change to use $$q, but that wouldn't do much for people who return `$q.reject() from interceptors

@caitp caitp force-pushed the coalesced-apply branch 2 times, most recently from 4ae9b8b to 3d0632c Compare August 22, 2014 22:45
@kseamon
Copy link
Contributor

kseamon commented Aug 22, 2014

Can you explain your last paragraph. What do you mean by last interceptor?

Sent from my iPhone

On Aug 22, 2014, at 6:29 PM, Caitlin Potter notifications@github.com wrote:

@IgorMinar PTAL --- I've omitted the "configurable coalesced apply wait time" because I couldn't really tell if you were saying use it or don't use it in the message on hangouts --- plus it didn't look like there was a real benefit to using it in testing.

So, one of the things that kind of throws a rock in this is the way we send the request as the last interceptor, so we get a full $digest anyways because of $evalAsync. This means each request is going to have a minimum of one digest all on its own, regardless of coalesced apply.


Reply to this email directly or view it on GitHub.

@caitp
Copy link
Contributor Author

caitp commented Aug 23, 2014

The request is actually sent after all of the request interceptors are run (unless we end up with a rejection before sending the request) --- that's what I meant. "last" in terms of "after all of the request interceptors are run"

@IgorMinar IgorMinar modified the milestones: 1.3.0, 1.3.0-beta.20 Aug 25, 2014
* @name $httpProvider#coalesceApply
* @description
*
* Configure $http requests to coalesce calls to $apply() for requests which are loaded close
Copy link
Contributor Author

Choose a reason for hiding this comment

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

responses which are received

Configure $http service to combine processing of multiple http responses received at around the same time via a $rootScope.$applyAsync. This can result in significant performance improvement for bigger applications that make many http requests concurrently (common during application bootstrap).

Defaults to false. If no value is specified, returns the current configured value.

@caitp
Copy link
Contributor Author

caitp commented Aug 26, 2014

You know what, I can't really remember why $evalAsync() wasn't good enough

@caitp caitp force-pushed the coalesced-apply branch 2 times, most recently from 03c2a4b to 5476a14 Compare August 27, 2014 00:14
caitp added 2 commits August 26, 2014 21:33
… $apply into a single digest.

It is now possible to queue up multiple expressions to be evaluated in a single digest using
$applyAsync. The asynchronous expressions will be evaluated either 1) the next time $apply or
$rootScope.$digest is called, or 2) after after the queue flushing scheduled for the next turn
occurs (roughly ~10ms depending on browser and application).
When multiple responses are received within a short window from each other, it can be wasteful to
perform full dirty-checking cycles for each individual response. In order to prevent this, it is
now possible to coalesce calls to $apply for responses which occur close together.

This behaviour is opt-in, and the default is disabled, in order to avoid breaking tests or
applications.

In order to activate coalesced apply in tests or in an application, simply perform the following
steps during configuration.

   angular.module('myFancyApp', []).
     config(function($httpProvider) {
       $httpProvider.useApplyAsync(true);
     });

OR:

   angular.mock.module(function($httpProvider) {
     $httpProvider.useApplyAsync(true);
   });
@caitp caitp closed this in ea6fc6e Aug 27, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants