sc-executor: Increase maximum instance count #1856
Conversation
It was reported that the maximum instance count of `32` isn't enough. By default `wasmtime` uses an instance count of `1000`. So, it should be safe to increase the instance count to `1000`.
|
Should fix: moonbeam-foundation/moonbeam#2509 |
|
Thank you Basti. |
|
Hmm yeah, actually this is already failing :P I had thought that this is only about reserved memory. |
|
Changes the value. I probably should improve this even more and just let it block when we try to allocate more than |
There was a problem hiding this comment.
Yeah, ultimately it would be best to have a low limit (we most likely don't need to support more parallel executor instances than the machine has hardware threads; otherwise that'll just lower the overall throughput and increase the risk of running out of memory) and have it block if we have too many instances running in parallel.
| @@ -22,7 +22,7 @@ use std::str::FromStr; | |||
| /// Parameters used to config runtime. | |||
| #[derive(Debug, Clone, Args)] | |||
| pub struct RuntimeParams { | |||
| /// The size of the instances cache for each runtime. The values higher than 256 are illegal. | |||
| /// The size of the instances cache for each runtime. The values higher than 32 are illegal. | |||
There was a problem hiding this comment.
Nit. what about a public const for 32?
There was a problem hiding this comment.
Yeah I was thinking about this as well. But then we would need to put this to some random place again or import another crate. So, I decided against this for now.
| if *counter + 1 < MAX_INSTANCE_COUNT { | ||
| *counter += 1; | ||
| } else { | ||
| self.wait_for_instance.wait(&mut counter); | ||
| *counter += 1; | ||
| } |
There was a problem hiding this comment.
Isn't there a possibility for a race condition here?
Consider the following scenario:
- Thread
Aacquires the lock. Findscounter == MAX_INSTANCE_COUNT. Waits in the counter condvar. - Some other thread (in the
dropabove) decrements the counter value and callsnotify_one(note that we release the lock before sending the notification) - Before thread
Ais woken up another threadBgrabs the lock and the counter goes back toMAX_INSTANCE_COUNT. - Thread
Anow receives the notification and acquires the lock. Increments the counter to be equal toMAX_INSTANCE_COUNT + 1
Maye this is safer?
while *counter >= MAX_INSTANCE_COUNT {
self.wait_for_instance.wait(&mut counter);
}
*counter += 1;
Or maybe release the lock in the drop after the notification is sent. But I don't know if is guaranteed that no other thread (not one in the wait queue) can grab the lock (alter the counter and release the lock) before the notified one is started.
There was a problem hiding this comment.
Hm, actually, I think you are right here. Yeah, 👍 to just have a loop here.
Co-authored-by: Koute <koute@users.noreply.github.com>
Co-authored-by: Koute <koute@users.noreply.github.com>
…ease-instance-count
Changes the maximum instances count for `wasmtime` to `64`. It also allows to only pass in maximum `32` for `--max-runtime-instances` as `256` was way too big. With `64` instances in total and `32` that can be configured in maximum, there should be enough space to accommodate for extra instances that are may required to be allocated adhoc. --------- Co-authored-by: Koute <koute@users.noreply.github.com>
Changes the maximum instances count for
wasmtimeto64. It also allows to only pass in maximum32for--max-runtime-instancesas256was way too big. With64instances in total and32that can be configured in maximum, there should be enough space to accommodate for extra instances that are may required to be allocated adhoc.