larger default stack size

vorner / corona

Coroutine and async/await support for tokio-based futures

Apache License 2.0

119 stars 8 forks source link

larger default stack size #7

Open GallagherCommaJack opened 5 years ago

GallagherCommaJack commented 5 years ago

This issue is to figure out what size the stack should be by default - the current one is small enough that it's really easy to run into stack overflow issues.

My best guess at a sensible default is to just use the default Linux thread stack size so that we won't run into stack overflows for normal rust code, which AFAICT is 8mb.

vorner commented 5 years ago

I'm a bit reluctant to be that generous.

First, while the main thread starts with 8, other threads seem to start with 2: https://docs.rs/tokio-threadpool/0.1.14/tokio_threadpool/struct.Builder.html#method.stack_size https://doc.rust-lang.org/std/thread/#stack-size

Furthermore, one might prefer corona over threads for various reasons, I guess at least 3:

It is more comfortable than using threads, especially if something hands out futures.
There are too many of them and context switching would be too expensive.
There are too many of them and the virtual size used by the thread stacks would be too high.

In particular, I believe people usually understand coroutines to be cheaper than threads and I would like not to do a surprise in that regard. Furthermore, I usually used something like 32 pages which is 128kB and was fine.

So maybe something like 0.25MiB would be reasonable?

Other option would be to force the user to choose by the API, make it mandatory argument.

GallagherCommaJack commented 5 years ago

Other option would be to force the user to choose by the API, make it mandatory argument.

This seems like the best choice that doesn't involve too much work - being explicit where it's easy reminds the user that they can have stack overflows.

The other approach which is I think more correct but much more work is dynamically growing stacks, but I'm not too optimistic about that approach given the API of the context library.

vorner commented 5 years ago

This seems like the best choice that doesn't involve too much work - being explicit where it's easy reminds the user that they can have stack overflows.

That would, however, be a breaking change and some changes. Do you want to try and see how many changes would it need?

The other approach which is I think more correct but much more work is dynamically growing stacks, but I'm not too optimistic about that approach given the API of the context library.

I don't think that's a workable solution in Rust. Growing stacks would need compiler support,we are a library (or maybe some very very crazy tricks).

GallagherCommaJack commented 5 years ago

That would, however, be a breaking change and some changes. Do you want to try and see how many changes would it need?

Eventually, yes, but for now I'm just going to work on the multicore feature.