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 leaks for token composition #2

Open
2 tasks
bergus opened this issue Sep 7, 2016 · 5 comments
Open
2 tasks

Fix memory leaks for token composition #2

bergus opened this issue Sep 7, 2016 · 5 comments

Comments

@bergus
Copy link
Owner

bergus commented Sep 7, 2016

See the case made by @stefanpenner in tc39/proposal-cancelable-promises#52

In many situations, a long-lived cancellation token (passed in by the caller) is combined with a short-lived token, e.g. in the "last" example or for a timeout:

function withTimeout(action, t) {
    return function(...args) {
        const token = args.pop();
        const timeout = new CancelToken(cancel => setTimeout(cancel, t, "timeout"));
        return action.call(this, ...args, token.concat(timeout));
    };
}

When the action is long over, and the combined token is no longer relevant to anyone, the token still holds are reference to it.
Todo:

  • ensure that it is dropped, or at least not more than a constant amount of references are kept (though not ideal for tree-like dependencies).
  • write refcounting test
@stefanpenner
Copy link

though not ideal for tree-like dependencies

I suspect this is a pretty common case. (in my travels, it appears to be)

@bergus
Copy link
Owner Author

bergus commented Sep 7, 2016

Actually the timeout example is a bad one, as the short-lived token does reliably get cancelled here and the composed one with it, so there's nothing to cling on any more. If I remember correctly, Creed's implementation of race/concat already removes all references from the sources when the result is cancelled by any.

The real problem is when the composed token is does not get cancelled for a long time and does have no handlers (that do anything) registered. Some of their short-lived source tokens might even have been garbage-collected already, but they are still registered on some long-lived root token. In that case, we would want the single dependency to be collapsed, and the reference from the root to the composed token to be removed.

@bergus
Copy link
Owner Author

bergus commented Sep 7, 2016

@stefanpenner Yeah, in general they always form tree-like dependency structures (or actually, a multitree), but they usually have a constant-bounded depth, right? So if we can keep the width (number of children, i.e. handlers) of each node constant at least, we should achieve overall constant memory.
I've made that distinction because we probably won't be able to prune the whole graph every time and still achieve necessary performance. I've successfully used similar tricks before.

Maybe you can share some examples where a large number of tokens is composed from a long-living one?

@stefanpenner
Copy link

@bergus would love to respond with some examples, but currently tied up with other stuff. If i don't respond by this upcoming monday, please feel free to pester me :)

@bergus
Copy link
Owner Author

bergus commented Sep 13, 2016

@stefanpenner Seems the time for pestering has come :-) Would you like a daily interval of inquiries?

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

No branches or pull requests

2 participants