Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WasmPageAllocator #3830

Merged
merged 20 commits into from
Dec 10, 2019
Merged

WasmPageAllocator #3830

merged 20 commits into from
Dec 10, 2019

Conversation

fengb
Copy link
Contributor

@fengb fengb commented Dec 2, 2019

Closes #3789

  • fuzz test
  • figure out large alignment tests — I turned off the alignment tests for wasm and updated the other semi-unrelated failures (giving 800K to FBA). The existing PageAllocator handles large alignment, but we've also discussed potentially simplifying them to only be page aligned.
  • optimize scan algorithm
  • import __heap_base properly — this is an internal detail of LLVM, plus having variable page size leads to unpredictable performance and behavior
  • optimize binary size

@fengb fengb force-pushed the wasm-page-allocator branch 2 times, most recently from aedfa7c to 036b90f Compare December 3, 2019 01:32
@fengb fengb force-pushed the wasm-page-allocator branch from 036b90f to 45e0441 Compare December 3, 2019 04:04
@fengb
Copy link
Contributor Author

fengb commented Dec 3, 2019

Did a bit of a spirit journey for the scanning algorithm: https://github.com/fengb/bitscan

It seems like naive iteration + a bit of block skipping is the fastest and smallest, and has more opportunities for bit tuning.

Length Naive Skip Swar64 Swar128
1 152681 597 857 684
2 160870 699 1445 1747
4 153152 564 2331 2998
8 150868 562 2568 3947
16 151038 640 3707 5333
32 164559 627 3491 6148

@fengb fengb changed the title [wip] WasmPageAllocator WasmPageAllocator Dec 6, 2019
@@ -763,7 +872,7 @@ test "ArenaAllocator" {
try testAllocatorAlignedShrink(&arena_allocator.allocator);
}

var test_fixed_buffer_allocator_memory: [80000 * @sizeOf(u64)]u8 = undefined;
var test_fixed_buffer_allocator_memory: [800000 * @sizeOf(u64)]u8 = undefined;
Copy link
Contributor Author

@fengb fengb Dec 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This broke alignment because 32 * page_size is a lot bigger in wasm land.

try testAllocatorAlignedShrink(allocator);
if (!std.Target.current.isWasm()) {
try testAllocatorLargeAlignment(allocator);
try testAllocatorAlignedShrink(allocator);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tests a new alignment of 32 * page_size.

@fengb fengb force-pushed the wasm-page-allocator branch from c12b5cb to e91522b Compare December 6, 2019 23:27
@@ -721,12 +802,46 @@ test "c_allocator" {
}
}

test "WasmPageAllocator internals" {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized this test can easily fail if any other file depends on page_allocator. Unfortunately, it looks pretty deep into the internal (unresetable) global state to make sure things are sane.

I have a few ideas to make this less dependent on existing behavior, but it'll have to assume total control of all global memory. Maybe that's okay?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think PageAllocator assuming total control of all wasm-intrinsically allocated memory is a reasonable design choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rewrote these tests to have much fewer assumptions on the internals. There's a hard dependency on ~270 MB of execution memory but I don't think it's too bad since it'll be reused.

@fengb
Copy link
Contributor Author

fengb commented Dec 9, 2019

A few thoughts related to tuning:

  1. Lowering the conventional memory used can drastically speed up tests (2.3s @ ~134 MB => 0.6s @ ~34 MB)
  2. Removing the tracking algorithm for alloc / free saves ~1 KB. It would be amazing if we could automatically detect whether free is used and adjust accordingly.

@andrewrk
Copy link
Member

andrewrk commented Dec 9, 2019

Removing the tracking algorithm for alloc / free saves ~1 KB. It would be amazing if we could automatically detect whether free is used and adjust accordingly.

One option is to support a config option in the root source file. Idea being you could opt out of free, making the implementation of free a no-op. With #2292, this would set shrinkFn to null.

@andrewrk andrewrk merged commit cd4d638 into ziglang:master Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WasmPageAllocator improvements
2 participants