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

$http should coalesce calls to $apply #5297

Closed
kseamon opened this issue Dec 5, 2013 · 30 comments
Closed

$http should coalesce calls to $apply #5297

kseamon opened this issue Dec 5, 2013 · 30 comments

Comments

@kseamon
Copy link
Contributor

kseamon commented Dec 5, 2013

Presently, $http calls $apply for every request response, even if they come in within milliseconds of each other. Using a short timeout (0ms, which in practice works out to 0-10ms) to $apply can reduce a lot of waste.

See example implementation here: https://github.com/kseamon/angular.js/compare/http-coalesce?expand=1

Using this code (Not ready for a pull request as it breaks tests and mocks, etc), I see the number of $apply calls in my app's startup drop from 40 to 16.

@ghost ghost assigned jeffbcross Dec 5, 2013
@IgorMinar
Copy link
Contributor

@cmelion from HBO is also interested in a solution like this.

@kseamon
Copy link
Contributor Author

kseamon commented Dec 6, 2013

The PR I'm working on introduces an $$applyAsync function to $rootScope. Calling it results in a timeout that will call apply. Repeated calls of it are coalesced into a single $apply.

For starters, I'm calling it from $http, $evalAsync, and $cookies.

@kseamon
Copy link
Contributor Author

kseamon commented Dec 6, 2013

I'll probably have it use a 0ms timeout. Increasing it does result in fewer apply calls in practice, but in my app, there's not much value to going above 10ms. I'd be fine using that too, but at some point we're slowing things down more than speeding them up.

@timkindberg
Copy link
Contributor

+1

14 similar comments
@kadimisetty
Copy link

+1

@dtabuenc
Copy link
Contributor

+1

@fwielstra
Copy link
Contributor

+1

@cesarandreu
Copy link

+1

@pixelcort
Copy link

+1

@erwinmombay
Copy link

+1

@damien-quintal
Copy link

+1

@codef0rmer
Copy link
Contributor

+1

@PatrickJS
Copy link
Member

+1

@just-boris
Copy link
Contributor

+1

@Fjandin
Copy link

Fjandin commented Dec 16, 2013

+1

@timelf123
Copy link

+1

@MattWalker
Copy link

+1

@greglockwood
Copy link

+1

@IgorMinar
Copy link
Contributor

@kseamon since this proposal was already approved for 1.3 you don't need to prefix it with $$ use $ in your PR instead.

@rynz
Copy link

rynz commented Dec 18, 2013

+1

4 similar comments
@isamulenko
Copy link

+1

@aroop
Copy link

aroop commented Dec 20, 2013

+1

@jkerkhofs
Copy link

+1

@saydogan
Copy link

+1

@thebigredgeek
Copy link
Contributor

I'm willing to tackle this, if we are okay going ahead with it @IgorMinar ?

@kseamon
Copy link
Contributor Author

kseamon commented Jan 23, 2014

I've got a PR in progress that I have not had a chance to work on in a while.

If you want to start from scratch, make sure to cover these points:

Implement $applyAsync on $rootScope.

Make $http and $cookies use it.

Make sure that regular $apply calls flush the queue and cancel the timeout.

@auser auser modified the milestones: Backlog, 1.3.0 Mar 17, 2014
@auser auser self-assigned this Mar 17, 2014
@IgorMinar IgorMinar modified the milestones: 1.3.0, Backlog May 20, 2014
@caitp caitp assigned caitp and unassigned auser May 27, 2014
@abirmingham
Copy link

+1

1 similar comment
@filso
Copy link

filso commented Jun 25, 2014

+1

@Narretz
Copy link
Contributor

Narretz commented Jun 25, 2014

@kseamon You could also push the branch to Github, if it's usable for further work.

@timelf123
Copy link

ping @kseamon

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