wasmtime / PR #2736 Add resource limiting to the Wasmtime... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #2736 Add resource limiting to the Wasmtime...

Wasmtime GitHub notifications bot (Mar 17 2021 at 23:52):

peterhuene opened PR #2736 from limiter to main:

This PR adds a ResourceLimiter trait to the Wasmtime API.

When used in conjunction with Store::new_with_limiter, this can be used to
monitor and prevent WebAssembly code from growing linear memories and tables.

This is particularly useful when hosts need to take into account host resource
usage to determine if WebAssembly code can consume more resources.

A simple StaticResourceLimiter is also included with these changes that will
simply limit the size of linear memories or tables for all instances created in
the store based on static values.

Wasmtime GitHub notifications bot (Mar 17 2021 at 23:54):

peterhuene requested pchickey for a review on PR #2736.

Wasmtime GitHub notifications bot (Mar 18 2021 at 00:51):

peterhuene updated PR #2736 from limiter to main.

Wasmtime GitHub notifications bot (Mar 18 2021 at 00:53):

peterhuene updated PR #2736 from limiter to main.

Wasmtime GitHub notifications bot (Mar 18 2021 at 19:47):

peterhuene updated PR #2736 from limiter to main.

Wasmtime GitHub notifications bot (Mar 18 2021 at 20:44):

peterhuene updated PR #2736 from limiter to main.

Wasmtime GitHub notifications bot (Mar 19 2021 at 15:50):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2021 at 15:50):

alexcrichton created PR Review Comment:

If this ends up being stored in individual tables/memories then this could probably turn into &'a Arc<dyn ...>

Wasmtime GitHub notifications bot (Mar 19 2021 at 15:50):

alexcrichton created PR Review Comment:

To make sure I understand this right, if any maximum is in play then this field is set to the minimum of all those maximums? The sources of maximum are:

The actual wasm type itself

The pooling allocator's configured maximum

A custom implementation of RuntimeMemory has a configured maximum

If my understanding is right, could this be clarified a bit? Basically None is only returned if literally nothing is limiting the memory growth, but otherwise Some doesn't imply that the wasm type itself had a maximum listed, just that something else somewhere is placing a constraint.

Also, presumably, this function isn't called unless desired is already known to be less than maximum, right? (or is it called even in those cases?)

Wasmtime GitHub notifications bot (Mar 19 2021 at 15:50):

alexcrichton created PR Review Comment:

Would it be possible to store the limiter into the Memory itself rather than in the Instance?

Wasmtime GitHub notifications bot (Mar 19 2021 at 15:50):

alexcrichton created PR Review Comment:

Could the docs here clarify that this is about the runtime limit maximum of the memory, not the maximum size as prescribed by the type in wasm?

Wasmtime GitHub notifications bot (Mar 19 2021 at 15:50):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2021 at 15:50):

alexcrichton created PR Review Comment:

This structure looks ripe for expansion to limiting other resources in the future, so perhaps this could use a builder-api pattern to avoid future API breakage?

Wasmtime GitHub notifications bot (Mar 19 2021 at 15:50):

alexcrichton created PR Review Comment:

I think this needs to be specified? Otherwise are memories created via Memory::new (or tables) limited via the limiter?

Wasmtime GitHub notifications bot (Mar 19 2021 at 15:50):

alexcrichton created PR Review Comment:

It looks like this value doesn't change over time, so I don't think it needs to be backed by a RefCell?

Wasmtime GitHub notifications bot (Mar 19 2021 at 15:50):

alexcrichton created PR Review Comment:

To avoid double-allocations and double-indirection could ResourceLimiterProxy have a type variable perhaps?

Wasmtime GitHub notifications bot (Mar 19 2021 at 18:18):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2021 at 18:18):

peterhuene created PR Review Comment:

Your understanding is correct and I can expand upon it in the docs for clarity.

The function is called even when desired exceeds the maximum as some implementers will want to know when the maximum was reached even if it was not directly limited by the limiter.

If that feels a little strange, perhaps a different function with the intention that it is only called when the memory couldn't grow due to it exceeding the maximum (from wasm / pooling allocator / custom runtime memory)?

Wasmtime GitHub notifications bot (Mar 19 2021 at 18:18):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2021 at 18:18):

peterhuene created PR Review Comment:

Good idea. I'll implement a builder.

Wasmtime GitHub notifications bot (Mar 19 2021 at 18:20):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2021 at 18:20):

peterhuene created PR Review Comment:

Good point. I'll fix.

Wasmtime GitHub notifications bot (Mar 19 2021 at 18:21):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2021 at 18:21):

peterhuene created PR Review Comment:

It currently does as it needs to sit on inner which is behind Rc and there's a circular reference between the proxy and the Store, which is why there is ugliness in Store::new_with_limiter to set this thing.

I'm not happy about it at all, so if there's a better way to do this such that the limiter can pass the store along in the callbacks, I'm definitely open to it.

Wasmtime GitHub notifications bot (Mar 19 2021 at 18:24):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Mar 19 2021 at 18:25):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2021 at 18:25):

peterhuene created PR Review Comment:

I will expand these docs.

Wasmtime GitHub notifications bot (Mar 19 2021 at 18:27):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2021 at 18:27):

peterhuene created PR Review Comment:

Can do.

Wasmtime GitHub notifications bot (Mar 19 2021 at 18:28):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Mar 19 2021 at 18:28):

peterhuene created PR Review Comment:

I'll change this, no reason to clone it for the request until a memory / table needs it.

Wasmtime GitHub notifications bot (Mar 22 2021 at 14:40):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 22 2021 at 14:40):

alexcrichton created PR Review Comment:

Ah ok nah I think for now this is fine. It runs a possible risk of making it difficult to optimize memory.grow or table.grow in the future but I don't think that's going to be the bottleneck of anything for a long time.

Wasmtime GitHub notifications bot (Mar 23 2021 at 01:37):

peterhuene edited PR #2736 from limiter to main:

This PR adds a ResourceLimiter trait to the Wasmtime API.

When used in conjunction with Store::new_with_limiter, this can be used to
monitor and prevent WebAssembly code from growing linear memories and tables.

This is particularly useful when hosts need to take into account host resource
usage to determine if WebAssembly code can consume more resources.

A simple Limiter is also included with these changes that will
simply limit the size of linear memories or tables for all instances created in
the store based on static values.

Wasmtime GitHub notifications bot (Mar 23 2021 at 21:13):

peterhuene updated PR #2736 from limiter to main.

Wasmtime GitHub notifications bot (Mar 23 2021 at 21:26):

peterhuene updated PR #2736 from limiter to main.

Wasmtime GitHub notifications bot (Apr 02 2021 at 18:36):

peterhuene updated PR #2736 from limiter to main.

Wasmtime GitHub notifications bot (Apr 02 2021 at 18:39):

peterhuene has marked PR #2736 as ready for review.

Wasmtime GitHub notifications bot (Apr 02 2021 at 23:00):

peterhuene requested alexcrichton and pchickey for a review on PR #2736.

Wasmtime GitHub notifications bot (Apr 05 2021 at 15:52):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 05 2021 at 15:52):

alexcrichton created PR Review Comment:

Arc is used here but I think Rc may suffice? (I didn't see any location that seemed to require thread-safety here)

Wasmtime GitHub notifications bot (Apr 05 2021 at 15:52):

alexcrichton created PR Review Comment:

One alternative design perhaps is that the Store isn't passed as an argument here and users manually RefCell and Weak themselves with the Store. Do you think, though, that this will be commonly desired such that we may as well go ahead and expose it?

Wasmtime GitHub notifications bot (Apr 05 2021 at 15:52):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 05 2021 at 15:52):

alexcrichton created PR Review Comment:

Hm ok, I had a question above about the Store argument and whether we can possibly remove that, but also here do you think it would make sense to remove the Option to effectively always have a limiting strategy?

Wasmtime GitHub notifications bot (Apr 05 2021 at 15:52):

alexcrichton created PR Review Comment:

Could the tests here be expanded with tests where the initial memory/table size exceed the maximum? Additionally API requests like Memory::new and Table::new should fail as well if they exceed the maximum? (ideally also tests where API-created objects will also fail to grow past the maximum)

Wasmtime GitHub notifications bot (Apr 08 2021 at 22:18):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 08 2021 at 22:18):

peterhuene created PR Review Comment:

Okay, getting back to this PR: I guess we don't have to expose the Store and let them capture whatever context they want since it's a custom implementation anyway.

I'll remove the Store parameter and that'll simplify things a bit.

Wasmtime GitHub notifications bot (Apr 09 2021 at 01:49):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 09 2021 at 01:49):

peterhuene created PR Review Comment:

The limiter needs to be an Option in the runtime for both table and memory so that the pooling allocator is able to "take" the memory/table when deallocating the instance (swaps in None).

I don't think having it also Option in Store makes it that much worse and it does have the benefit of not boxing a new trait object every time a Store is created just to return true on memory/table grow operations.

If you're okay with it, I'd like to leave this one as-is.

Wasmtime GitHub notifications bot (Apr 09 2021 at 01:49):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Apr 09 2021 at 02:04):

peterhuene updated PR #2736 from limiter to main.

Wasmtime GitHub notifications bot (Apr 09 2021 at 02:04):

peterhuene requested alexcrichton and pchickey for a review on PR #2736.

Wasmtime GitHub notifications bot (Apr 09 2021 at 02:16):

peterhuene updated PR #2736 from limiter to main.

Wasmtime GitHub notifications bot (Apr 15 2021 at 14:18):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2021 at 14:18):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2021 at 14:18):

alexcrichton created PR Review Comment:

To confirm, are there tests for this? It seems a bit fishy that we're deallocating a half-initialized instance without many modifications below. I'd expect, for example decommit_memory_pages to fail one one way or another if called on an othewise unmapped region?

Wasmtime GitHub notifications bot (Apr 15 2021 at 14:18):

alexcrichton created PR Review Comment:

I suppose one other possible option is to store *mut dyn ResourceLimiter in the *mut VMContext so we can use Box here instead of Rc, but let's only do that if the need actually arises.

Wasmtime GitHub notifications bot (Apr 15 2021 at 14:18):

alexcrichton created PR Review Comment:

Could the documentation here be expanded to indicate that the created Store has no limits on usage of memory and such in wasm modules?

Wasmtime GitHub notifications bot (Apr 15 2021 at 14:18):

alexcrichton created PR Review Comment:

Could this link to the new_with_limits API?

Wasmtime GitHub notifications bot (Apr 15 2021 at 14:18):

alexcrichton created PR Review Comment:

Could the documentation here link to the ResourceLimiter trait which discussed which limits can be configured?

Wasmtime GitHub notifications bot (Apr 15 2021 at 14:18):

alexcrichton created PR Review Comment:

Oh also, could this link to StoreLimits and StoreLimitsBuilder for a built-in way to limit resources?

Wasmtime GitHub notifications bot (Apr 16 2021 at 20:17):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 16 2021 at 20:17):

peterhuene created PR Review Comment:

The test_pooling_allocator_initial_limits_exceeded test does cover this, albeit with empty memory/table lists on deallocate, as that test is what caught the problem: with an instance limit of 1, it failed to allocate any more instances because it wasn't being added back to the free list. I will add another memory to the module definition such that the list of initialized memories isn't empty, just in case.

While the instance isn't completely initialized, it should be safe to call deallocate on it as we iterate back through only initialized Memory and Table to call decommit_memory_pages on pages that were either made accessible (for Memory) or explicitly committed (for Table) prior to inserting into those lists.

Wasmtime GitHub notifications bot (Apr 16 2021 at 22:56):

peterhuene updated PR #2736 from limiter to main.

Wasmtime GitHub notifications bot (Apr 16 2021 at 22:57):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 16 2021 at 22:57):

peterhuene created PR Review Comment:

:+1: I'm going to leave this as-is and we can revisit in the the future if needed.

Wasmtime GitHub notifications bot (Apr 19 2021 at 14:19):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 19 2021 at 14:19):

alexcrichton merged PR #2736.

Last updated: Nov 22 2024 at 17:03 UTC