Better stack limits #554
Better stack limits #554
Conversation
magik6k
force-pushed
the
fix/native-stack-limits
branch
from
aab7cba
to
43c03bc
Compare
| @@ -38,9 +39,24 @@ impl<K: Kernel> DerefMut for DefaultExecutor<K> { | |||
| } | |||
| } | |||
|
|
|||
| lazy_static! { | |||
| static ref EXEC_POOL: yastl::Pool = yastl::Pool::with_config( | |||
| 8, | |||
There was a problem hiding this comment.
This is an arbitrary number; Just for chain-sync we probably don't need more than 1/2, but some users may want this to be higher e.g. to get internal sends in a bunch of historic tipsets.
There was a problem hiding this comment.
Some people have a lot of CPUs. We're already reserving half a gig for stacks here. We'll actually likely want to shrink this.
There was a problem hiding this comment.
I.e., 128 cores = 8GiB of memory just sitting there.
magik6k
force-pushed
the
fix/native-stack-limits
branch
2 times, most recently
from
17d75b5
to
bd9c6ab
Compare
magik6k
force-pushed
the
fix/native-stack-limits
branch
from
bd9c6ab
to
4811526
Compare
There was a problem hiding this comment.
Looks good! Just a few questions for now. Review from @Kubuxu @dignifiedquire @Stebalien @mriise would be welcome.
| @@ -124,8 +124,8 @@ impl NetworkConfig { | |||
| pub fn new(network_version: NetworkVersion) -> Self { | |||
| NetworkConfig { | |||
| network_version, | |||
| max_call_depth: 4096, | |||
| max_wasm_stack: 64 * 1024, | |||
| max_call_depth: 1024, | |||
There was a problem hiding this comment.
To be technically correct, we should set 1024 on nv16 and above, only.
There was a problem hiding this comment.
So, @magik6k pointed out that we'll hit the rust stack limit at 4096. The only real solution would be to increase the thread size.
But, in practice, it's unclear if one can actually make a message that can get past 1024 calls on mainnet. By "unclear", I mean "totally not close to possible". @magik6k only got to 1025 by making a custom actor that did nothing but recursively send. On mainnet, one would have to make a recursive call in a multisig actor, where each recursive call would be significantly more expensive (running out of block gas before anything else).
There was a problem hiding this comment.
Tradeoffs:
- The current way, it's unlikely that something goes wrong, and the worst-case is a fork (for anyone running with the FVM_EXPERIMENTAL flag).
- By increasing to 4096 (without changing the stack size), it's also unlikely that something will go wrong, but we'll crash (hard) if we do go past 1024.
testing/integration/tests/lib.rs
Outdated
| .execute_message(message, ApplyKind::Explicit, 100) | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(res.msg_receipt.exit_code.value(), 0) |
There was a problem hiding this comment.
Is the exit code you see here 10?
There was a problem hiding this comment.
Added more cases with more readable codes.
magik6k
force-pushed
the
fix/native-stack-limits
branch
2 times, most recently
from
c4a7c8f
to
8f5fa91
Compare
magik6k
force-pushed
the
fix/native-stack-limits
branch
from
8f5fa91
to
1ad79bf
Compare
| @@ -38,9 +39,24 @@ impl<K: Kernel> DerefMut for DefaultExecutor<K> { | |||
| } | |||
| } | |||
|
|
|||
| lazy_static! { | |||
| static ref EXEC_POOL: yastl::Pool = yastl::Pool::with_config( | |||
| 8, | |||
There was a problem hiding this comment.
Some people have a lot of CPUs. We're already reserving half a gig for stacks here. We'll actually likely want to shrink this.
todo:
Fixes #552