wasm/alloc: zero fill memory #15063

rockwotj · 2023-11-21T03:56:52Z

Wasmtime assumes that memory returned from allocators is zero filled. We need to do this for our custom allocators too.

Backports Required

Release Notes

none

This is assumed by wasmtime for these memory APIs Signed-off-by: Tyler Rockwood <[email protected]>

rockwotj · 2023-11-21T03:59:20Z

Force push: Fix release build

rockwotj · 2023-11-21T15:04:45Z

Force push: Remove a debug build check that was causing asan false positives

rockwotj · 2023-11-21T16:51:49Z

src/v/wasm/allocator.h

+     * don't have to touch all the bytes.
     */
-    void deallocate(heap_memory);
+    void deallocate(heap_memory, size_t used_amount);


This amount is user configurable and in theory could be 10s of MB, I guess that is too expensive to memset in a single go? Do we need to do this in an async friendly way?

Like chunk-wise with yields inerspersed or something?

Does N have a concrete upper bound?

4GiB, probably in practice it's much lower (the default is 2MiB), but for example golang (not tinygo) is requiring 50MB in my tests for a non trivial program (seems like a lot but 🤷)

I have an idea to do this in a fairly lightweight way, but I will do in a followup PR, not this one.

i would assume that as long as memset(0 is on a contiguous region it is very fast. i dunno tho what the limit is where it becomes costly enough to be a concern.

My suggestion is to get this in, then I'll create a benchmark

It's also page aligned, that should help

oleiman

lgtm

oleiman · 2023-11-21T23:32:40Z

src/v/wasm/allocator.h

+     * don't have to touch all the bytes.
     */
-    void deallocate(heap_memory);
+    void deallocate(heap_memory, size_t used_amount);


Like chunk-wise with yields inerspersed or something?

oleiman · 2023-11-21T23:55:27Z

src/v/wasm/tests/wasm_allocator_test.cc

+    allocated = allocator.allocate(req);
+    ASSERT_TRUE(allocated.has_value());
+    EXPECT_THAT(allocated, Optional(HeapIsZeroed()));


question: what's the purpose of this second allocation?

It's reused, so its ensured memory is zeroed when returned "dirty" to the allocator.

rockwotj · 2023-11-22T02:06:18Z

/ci-repeat

oleiman

might want input from noah regarding the big memset, but lgtm

vbotbuildovich · 2023-11-22T04:31:25Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/41544#018bf50b-15f3-49ce-8ef3-b8632c06513c

dotnwat

lgtm

dotnwat · 2023-11-22T04:35:31Z

src/v/wasm/allocator.cc


-void heap_allocator::deallocate(heap_memory m) {
+void heap_allocator::deallocate(heap_memory m, size_t used_amount) {
+    std::memset(m.data.get(), 0, std::min(used_amount, m.size));


defensive programming and all, yay. but it also reads as if more could have been used than the buffer size. is that a thing? maybe used_amount corresponds to an amount that perhaps spans instances of heap_memory? that would explain it i think

You're right, I should just remove the std::min it's overly defensive

dotnwat · 2023-11-22T04:37:49Z

src/v/wasm/allocator.h

+     * don't have to touch all the bytes.
     */
-    void deallocate(heap_memory);
+    void deallocate(heap_memory, size_t used_amount);


i would assume that as long as memset(0 is on a contiguous region it is very fast. i dunno tho what the limit is where it becomes costly enough to be a concern.

wasmtime assumes that memory is zero filled, so ensure that heap memory is zero filled. We also force callers to tell the allocator how much of that heap was actually used, so that deallocation can memset only the changed memory - this is an optimization for large memories if only part of the memory is used. Signed-off-by: Tyler Rockwood <[email protected]>

Adds tests that we ensure memory is always zero filled Signed-off-by: Tyler Rockwood <[email protected]>

rockwotj · 2023-11-22T15:16:28Z

Force push: Remove overly defensive cap to memset

oleiman

rubber stamp

rockwotj · 2023-11-22T16:34:16Z

@oleiman @dotnwat on my local machine:

test	iterations	median	mad	min	max	allocs	inst
Memset.Speed_20MiB	3323	299.810us	208.985ns	299.365us	300.527us	1.000	367.9
Memset.Speed_100MiB	719	1.383ms	4.891us	1.378ms	1.392ms	1.000	371.1
Memset.Speed_2MiB	64465	16.100us	9.519ns	15.781us	16.112us	1.000	361.0

Presumably arm is a little slower, so I suspect most initial transforms will be fine in terms of budget, but there are other things going in this task when we dealloc an instance, so I can make it async

wasm/alloc: zero-fill stack memory

feb4a06

This is assumed by wasmtime for these memory APIs Signed-off-by: Tyler Rockwood <[email protected]>

github-actions bot added the area/redpanda label Nov 21, 2023

rockwotj requested review from dotnwat and oleiman November 21, 2023 03:57

rockwotj self-assigned this Nov 21, 2023

rockwotj force-pushed the memset branch from 47445e2 to f64c828 Compare November 21, 2023 03:59

rockwotj force-pushed the memset branch from f64c828 to 2ef526d Compare November 21, 2023 15:04

rockwotj commented Nov 21, 2023

View reviewed changes

oleiman reviewed Nov 21, 2023

View reviewed changes

rockwotj requested a review from oleiman November 22, 2023 02:05

oleiman previously approved these changes Nov 22, 2023

View reviewed changes

dotnwat reviewed Nov 22, 2023

View reviewed changes

rockwotj added 2 commits November 22, 2023 06:55

wasm/alloc: tests for zero fill

b7846d7

Adds tests that we ensure memory is always zero filled Signed-off-by: Tyler Rockwood <[email protected]>

rockwotj dismissed oleiman’s stale review via b7846d7 November 22, 2023 12:55

rockwotj force-pushed the memset branch from 2ef526d to b7846d7 Compare November 22, 2023 12:55

rockwotj requested review from oleiman and dotnwat November 22, 2023 15:27

oleiman approved these changes Nov 22, 2023

View reviewed changes

rockwotj merged commit 35528be into redpanda-data:dev Nov 22, 2023

rockwotj deleted the memset branch November 22, 2023 16:21

rockwotj mentioned this pull request Nov 22, 2023

wasm: asynchronously zero deallocated heaps #15101

Merged

7 tasks

wasm/alloc: zero fill memory #15063

wasm/alloc: zero fill memory #15063

Uh oh!

Conversation

rockwotj commented Nov 21, 2023

Backports Required

Release Notes

Uh oh!

rockwotj commented Nov 21, 2023

Uh oh!

rockwotj commented Nov 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oleiman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rockwotj commented Nov 22, 2023

Uh oh!

oleiman left a comment

Choose a reason for hiding this comment

Uh oh!

vbotbuildovich commented Nov 22, 2023

Uh oh!

dotnwat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rockwotj commented Nov 22, 2023

Uh oh!

oleiman left a comment

Choose a reason for hiding this comment

Uh oh!

rockwotj commented Nov 22, 2023

Uh oh!

Uh oh!