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
Make recursive calls to flatMap stack safe #44
Conversation
Pull Request Test Coverage Report for Build 77
💛 - Coveralls |
Not sure why the second build failed, I only pushed a second test which passed. |
Also this would ideally have the interfaces merged as the trampoline should not really be public. See #42 |
I'm not sure we want to add the cost of a trampoline to every invocation of the Future library. Seems that we should think about it as an "opt-in" feature for cases where it would make sense. Elm has a library which does just that: https://github.com/elm-lang/trampoline/blob/master/src/Trampoline.elm |
The overhead of the trampoline is very low, for non nested calls it just creates a thunk and immediately invokes it and tries to pop an array, otherwise there is just an additional unshift. For anyone doing anything asynchronous this overhead is likely negligible, a test that runs 100k loops takes well under 1s. For anyone doing anything with synchronous futures this fixes a breaking stack overflow for large nesting. How would you use that trampoline library with futures? If you have a If you don't think the benefits of this feature outweigh the costs then fair enough but this will bite users of your library at some point if they are writing recursive functions using futures. |
It's not that I think or believe that the costs will outweigh the benefits, it's the fact that I do not know. Nor do I believe that we can understand the usage of such a low level library to make an all encompassing decision. This is why I made the suggestion that we allow the user of our library to decide for themselves when and if they would like to incur the cost of the additional overhead.
Running a single recursive loop tells us nothing about the user who is utilizing the Future library throughout their code base. We would be making a decision that extra allocation is ok for their use case.
I did not intend to suggest that we utilize the library or it's design pattern within the Future codebase. Merely showing that there are other ways to accomplish the task whereby we allow the user to decide when to incur the cost of the additional allocation. All of that being said, this really should be implemented as an unrolled while loop to make this debate unnecessary. |
I'm definitely not an expert in this so I'm not sure I fully follow but the fastest solution in http://glat.info/fext/#section_setup_details_speed_test looks almost identical to this. The only difference being that it will allow the stack to grow to 5 before trampolining. function runLoop(_callback) {
while(true) {
var callback = _callback;
Curry._1(callback, /* () */0);
var match = callbacks.pop();
if (match !== undefined) {
_callback = match;
continue ;
} else {
return /* () */0;
}
};
} If you have a solution to make this work currently please let me know but with the current implementation I couldn't find an easy way to make things stack safe. Or do you mean an option to allow opt in during make, i.e. provide an optional flag to use or not use trampoline? |
Anyway this PR isn't really going anywhere right now so I'll open an issue and link this PR. Then you can make a decision on what changes if any can be made. |
If you want to make your implementation available under a constructor flag then we bump the major version, I think that’s a good stop-gap for the stack explosion.
… On Dec 11, 2019, at 1:52 PM, Tom Mottram ***@***.***> wrote:
Anyway this PR isn't really going anywhere right now so I'll open an issue and link this PR. Then you can make a decision on what changes if any can be made.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#44?email_source=notifications&email_token=AABZG4GOJAQVRXVDNMSTYKDQYFOIRA5CNFSM4JYKQZX2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGUWJUA#issuecomment-564749520>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABZG4HEFYEBREZJYRVGFATQYFOIRANCNFSM4JYKQZXQ>.
|
Just want to make sure we don’t explode someone’s performance expectations.
… On Dec 11, 2019, at 1:52 PM, Tom Mottram ***@***.***> wrote:
Anyway this PR isn't really going anywhere right now so I'll open an issue and link this PR. Then you can make a decision on what changes if any can be made.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#44?email_source=notifications&email_token=AABZG4GOJAQVRXVDNMSTYKDQYFOIRA5CNFSM4JYKQZX2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGUWJUA#issuecomment-564749520>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABZG4HEFYEBREZJYRVGFATQYFOIRANCNFSM4JYKQZXQ>.
|
I pushed my latest changes now, I've kept backwards compatibility here. Let me know what you think of the API. I could also add another option to allow user specified executors other than trampoline type executor = [|`executor(('a, 'a => unit) => unit)] Again this would ideally be merged with the |
Will need to update docs + not sure if trampoline is intuitive enough, maybe `stackSafe? |
src/Future.re
Outdated
@@ -21,21 +51,23 @@ let make = resolver => { | |||
Future( | |||
resolve => | |||
switch (data^) { | |||
| Some(result) => resolve(result) | |||
| Some(result) => trampoline(() => resolve(result)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this mean the trampoline still runs on every invocation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes my mistake, will fix in next commit.
->Future.get(r => r |> expect |> toEqual(numberOfLoops) |> finish); | ||
}); | ||
|
||
testAsync("async recursion is stack safe", finish => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make a test using the default executor to ensure it blows the stack and we're not just always using the trampoline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See "value recursion blows the stack with default executor"
@@ -311,4 +311,38 @@ describe("Future Belt.Result", () => { | |||
|> finish | |||
); | |||
}); | |||
}); | |||
|
|||
testAsync("value recursion is stack safe", finish => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make a deeply nested chain and test that we don't blow the stack there. Something like the following:
next(x)
->Future.flatMap(x => Future.value(x + 1))
->Future.map(x => x + 1)
->Future.flatMap(x => Future.value(x + 1)->Future.flatMap(x => Future.value(x + 1)))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/Future.re
Outdated
@@ -1,17 +1,47 @@ | |||
type getFn('a) = ('a => unit) => unit; | |||
|
|||
type executorOptions = [ | `none | `trampoline]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe rename to executorType
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think trampoline
is good. If you run into the stack problem, trampoline
is something you'll definitely find right away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, great work
@tomp4l @briangorman Once #42 is merged then this is good to go |
Will need minor tweaks for the addition of the executor |
To make recursive algorithms easier to implement this pushes any nested callbacks in flatMap onto a 'trampoline' which means only one can run at any time. This is useful when synchronous futures are created and used in recursive functions.
Use polymorphic variants for ease of use and to support the option of passing a parameterised version in future with user specified executor Keeps backwards compatibility.
I've updated the rei file, one change I did make was to make the future type abstract as I think it's more useful than exposing the underlying implementation and will make avoiding breaking changes much easier in future. If you disagree I can add the actual implementation to the interface but it will be a breaking change. As you can see the changes otherwise to the rei are minimal with just addition of optional arguments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
To make recursive algorithms easier to implement this pushes any
nested callbacks in flatMap onto a 'trampoline' which means only one
can run at any time. This is useful when synchronous futures are
created and used in recursive functions.
Without this patch the following test failure would occur:
resolves #44