Skip to content
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

Fix memory leak #67

Merged
merged 4 commits into from Apr 3, 2015
Merged

Fix memory leak #67

merged 4 commits into from Apr 3, 2015

Conversation

@ForbesLindesay
Copy link
Member

@ForbesLindesay ForbesLindesay commented Dec 26, 2014

This is a crude attempt at fixing the memory leak caused by code like:

function next() {
  return new Promise(function (resolve) {
   setImmediate(resolve)
  }).then(next)
}
next()

In theory this should be a viable way of running a continuous polling operation.

@ForbesLindesay
Copy link
Member Author

@ForbesLindesay ForbesLindesay commented Dec 26, 2014

P.S. don't merge this yet. We should make "handle" some kind of private symbol if we are going to take this approach.

@stefanpenner
Copy link

@stefanpenner stefanpenner commented Dec 28, 2014

just for note, there is an observable difference that may or may not be tolerable.

promises-aplus/promises-spec#179 (comment)

@ForbesLindesay
Copy link
Member Author

@ForbesLindesay ForbesLindesay commented Dec 28, 2014

That's interesting. I think that probably is tolerable, but I do wonder if we could fix that somehow.

@edef1c
Copy link
Member

@edef1c edef1c commented Jan 5, 2015

@ForbesLindesay I've been contemplating doing this for a long time, and I probably have a local branch around doing a similar thing. Collapsing promise chains like this should be a nice perf boost.
My own plan was to stick a few private symbols on the then function, containing the state. Handlers could be offloaded onto that promise straight away, and synchronous inspection (from the then being invoked, instead of dispatching) becomes feasible too. I can't find my version of the code right now, but I have a feeling it'll be faster than invoking further dynamic dispatch.

@edef1c
Copy link
Member

@edef1c edef1c commented Jan 5, 2015

I think depending on es6-symbol and sticking everything under then should resolve privacy/encapsulation concerns. I'll have to run some perf tests on the cost of messing with function properties. I'm not sure how those'll turn out, but I'd rather not lose the semantic niceties of that to the Perf Monster.

@ForbesLindesay
Copy link
Member Author

@ForbesLindesay ForbesLindesay commented Jan 8, 2015

The encapsulation is just as good if we use es6-symbol on this rather than this.then so I would suggest sticking with that as it seems simpler.

@edef1c
Copy link
Member

@edef1c edef1c commented Jan 9, 2015

Sure, but .then can freely be moved around and replaced. We could get some very weird behaviour on subclasses. .then is where the behaviour of .then is defined, basically.

@ForbesLindesay ForbesLindesay force-pushed the fix-memory-leak branch from 5c2f0e4 to fa3a970 Apr 3, 2015
Forbes Lindesay added 2 commits Apr 3, 2015
This fixes problems with very nested promises without recreating the
memory leak
@ForbesLindesay
Copy link
Member Author

@ForbesLindesay ForbesLindesay commented Apr 3, 2015

I now believe this is ready to merge, and it fixes a major memory leak. @nathan7 if you can review this asap and do the honours, that would be great.

ForbesLindesay added a commit that referenced this pull request Apr 3, 2015
Fix memory leak
@ForbesLindesay ForbesLindesay merged commit e5ff259 into master Apr 3, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@ForbesLindesay ForbesLindesay deleted the fix-memory-leak branch Apr 3, 2015
@ForbesLindesay
Copy link
Member Author

@ForbesLindesay ForbesLindesay commented Apr 3, 2015

I'm merging this now to get unblocked, but I'd still love a review if someone gets a chance.

@edef1c
Copy link
Member

@edef1c edef1c commented Apr 4, 2015

Looks good to me, @ForbesLindesay! Clever trick with the internalHandle fn!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants