-
Notifications
You must be signed in to change notification settings - Fork 172
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
turn a few Lwt_list functions into a tail recursive variant #347
Conversation
src/core/lwt_list.ml
Outdated
let tx = f x and tl = iter_p f l in | ||
tx >>= fun () -> tl | ||
let iter_p f l = | ||
let ts = List.map f l in |
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.
List.map
is unfortunately not tail-recursive (see link). The same applies to List.mapi
. For map
, I think you can do something like List.rev (List.rev_map f l)
. For mapi
, I guess a custom helper is necessary.
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.
Right. Ok, I'll make some changes for this
.travis.yml
Outdated
@@ -38,3 +38,5 @@ notifications: | |||
- [email protected] | |||
on_success: always | |||
on_failure: always | |||
|
|||
sudo: required |
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 don't think this should be included in the PR.
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.
ok, I'll remove it. somehow I did need this to get travis to build & run the tests on my fork of the lwt repo...
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.
BTW, you also have the option to submit a PR, tell us not to review it, and use it to trigger CI in this repo :) Not sure why sudo: required
would be required (heh). Perhaps the main Lwt repo needs it now too, but we are squeezing by due to some legacy support from Travis...?
Thanks. I'm a bit surprised that these aren't tail-recursive to begin with. For |
You may want to read Leroy's classic "defense" of the non-tail-recursive |
That post makes a good case for the existence of both maps but it doesn't justify why the faster and more dangerous version should be the default. Similarly, I would be very interested to see an application that where |
Now let me actually contribute something to this discussion: Perhaps we can get some inspiration by base's map function which is both fast on small lists and tail recursive at the cost of being ugly: https://github.com/janestreet/base/blob/master/src/list.ml#L313-L345 |
How does his argument get stronger in the lwt case? The way I see it is different:
|
We would be even more inclined to optimize for short lists, because doing asynchronous things on long lists is likely to pose other challenges. But apparently there are counter-arguments. Ultimately, I have no objection to a tail-recursive implementation :). |
Just want to clarify, that if Lwt ends up waiting for I/O, the corresponding function returns the promise immediately, releasing the stack frame, etc. The stack space is therefore not used – Lwt will call the callback that continues the computation from that promise later, after the I/O completes, on a different stack frame – and that stack frame is likely to be shallower. It's still the same thread's stack, though. A few Lwt functions on Windows currently have blocking implementations, but for those, Lwt blocks the whole thread making the call anyway. None of the above is against tail recursion – it's just a clarification. |
Thanks for the correction/clarification @aantron. I haven't done any work yet regarding lack of tail recursiveness of List.map(i) functions. I could as suggested add some custom implementation for it, based on e.g. the code in janestreet base ... |
We could, and I'm considering it/have considered it in the past, for other reasons as well. But, I think for now, if there is no evidence that the performance of I'm assuming you hit an actual problem with these functions not being tail-recursive. So, I say let's solve that problem. We can use @rgrinberg's helpful comment separately later, as a reference, if we get feedback that these functions are too slow. |
Alternatively, we could add the "fastest map possible," and comment that it came from base, or wherever. If/when Lwt decides on an stdlib extension to depend on, the comment can tell us to eliminate that code in favor of the dependency, during a future refactoring. |
Can't we regularly crash the stack by using the scheduler and a judiciously designed wait/wakeup ? Like, let's say, every I tend to agree with @vasilisp though: the existing implementation is fairly good and while a tail-rec version might be desirable, it shouldn't compromise efficiency on small lists (which is probably the main usage pattern). |
We didn't have an actual problem with this ... we were looking for a segfault/stackoverflow, and in the end it turned out to be related to logging (and is fixed with #348). |
Yes, and I've hit this. There is a discussion about eliminating/discouraging/warning about that in #329. If there is no current need to change |
Also, FTR, I usually prefer tail recursion, even if the function is slower, unless there is evidence that's the bottleneck for enough users. I guess when I am a user, I'd rather not worry that safe-looking functions might one day suddenly trigger segfaults, just from me calling them on mystery inputs, or scaling my application. I'd much rather deal with a performance bottleneck, since by then I'm likely already set up for finding them (i.e. doing profiling and the like). But I guess this is best resolved on a case-by-case basis. |
Alright, I decided to just finish off this PR. I'm not comfortable with these functions not being tail-recursive. That's just a source of anxiety for users, IMO. @domsj I pushed an extra commit into your branch, to not use stdlib's We can optimize these functions later, if we get any evidence that they are a performance bottleneck. I left a comment pointing to the implementation in base linked by @rgrinberg. If necessary, we can use base as a guideline, or just use base. In the future, we should go through all of |
@aantron I think you misunderstood: I was thinking about that as a feature, in order to avoid stack overflow. It doesn't make the function tail rec, but it doesn't matter if the stack is reset regularly. Also, I'm sorry, but all the new versions are really much much slower. Good defaults are also about performances. You are very careful about semantic changes, I'm surprised it doesn't bother you to make basic Lwt functions several times slower. |
I think I did, thanks for pointing it out. I guess you mean forcing the loop through Lwt's queue once every so many iterations, which would indeed prevent stack overflow. That seems like a pretty funny hack, though. Maybe that's what we should do, but then, why the "was" in
It does bother me, but presumably, we can optimize tail-recursive functions for performance to some extent. Perhaps it is better to measure the various options first and do a survey? Perhaps the result should go into a blog post, or one is already available? At the same time, it's not clear to me that, in a bigger context, even if these functions are much slower, they will often become a bottleneck. As I said above, I'd much rather have someone decide that " In either case, the user is forced to write an alternative implementation (at least in the near term), but only the second one is graceless, catastrophic, and unpredictable. Avoiding that seems like a more basic need than performance. |
Also, I'm sorry, but all the new versions are really much much slower. Good defaults are also about performances. You are very careful about semantic changes, I'm surprised it doesn't bother you to make basic Lwt functions several times slower.
Very curious to see measurements if you've made some. I'm kind of surprised it's
that much slower in this case as well. With all the binds inserted everywhere.
I think that if the overhead is proven to be significant then we should
seriously consider on waiting the optimized versions to appear.
|
I think it's quite a good idea, actually. It's still done in batteries. Not sure about core. The main problem is that it's not type-safe ... but unlike batteries, we already have a mechanism for write-once references: At the least, it's worth a try, given it should have good properties wrt small lists. |
Note that you can mix direct style (for a max number of iterations) and then the safer, slower tailrec version. This is a good mix in my experience with normal blocking code on lists. I can show examples if you wish. |
@c-cube: this actually is for blocking code on lists, at least as written in the first commit in this PR. So, I guess whatever experience you have with that applies here. |
Before 3.1.0 (mid-July), we will find out which tail-recursive implementation is the fastest, or fast enough, switch to it, and ideally document it in an article or a blog post, so this work does not have to be done again for a while. Lwt APIs should not be a source of uncertainty for users. |
My experience is mostly playing with such functions in containers (where I provide safe variants of some List functions). map: https://github.com/c-cube/ocaml-containers/blob/master/src/core/CCList.ml#L23 the results of benchmarks on 4.04.0+flambda: https://paste.isomorphis.me/p0K&raw |
b8b3168
to
0773186
Compare
FWIW, I've done some experiments benchmarking different implementations of To summarize the relevant parts of that writeup, the data suggested that:
|
Thanks @jsthomas! My summary of the findings:
What I propose we do with this right now, however, is implement naive tail-recursion, and put a link in a comment by it to the message from @jsthomas. If we really have users for whom CPU/memory performance of |
We discussed further on IRC, and there are even more optimizations to potentially make. For now, I've rebased this PR to resolve merge conflicts, and we are merging the naive tail-recursive implementation. |
The annotation is added to functions that were modified in #347. It will be added to other functions in Lwt_list during a future thorough review of that module. [@ocaml.tailcall] was introduced in OCaml 4.03.0. On 4.02, the annotation is silently ignored, so it is safe. Working on 4.03.0 and higher is enough for any problems with tail recursion to be caught in CI.
Before this commit, if Lwt_switch.turn_off had multiple hooks to call, and one of the hooks raised an exception, the thread created by turn_off would not wait for the remaining hook threads to complete.
No description provided.