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

gc=custom build option to allow custom GC implementations #3278

Closed
anuraaga opened this issue Nov 8, 2022 · 6 comments
Closed

gc=custom build option to allow custom GC implementations #3278

anuraaga opened this issue Nov 8, 2022 · 6 comments
Labels
enhancement New feature or request wasm WebAssembly

Comments

@anuraaga
Copy link
Contributor

anuraaga commented Nov 8, 2022

In coraza-proxy-wasm, we are creating a wasm binary for use with Envoy in cases where performance matters, while code size doesn't so much. I've previously mentioned gaining a lot of performance by using mimalloc for malloc/free instead of delegating to the TinyGo heap for them. This involved -gc=none and swapping runtime.alloc with the same code as currently in gc_conservative.go, except using a fixed-size malloc'd buffer as the heap instead of taking control of the entire wasm heap.

Naturally this has issues of being a fixed size heap, and while performance was much better with the non-Go allocations off the Go heap (no 1s pauses), it still needed to be improved (~100ms pauses). So the next step was replacing the GC with one that isn't tiny, ideally without having to actually write one.

I succeeded in swaping out the GC with bdwgc compiled to wasm. Performance is much better, with pauses <10ms from what I can tell. It's a number that is now unnoticable when just running through integration test requests while previously they were clearly noticable.

https://github.com/corazawaf/coraza-proxy-wasm/compare/main...anuraaga:bdwgc?expand=1#diff-eb1422fbb127355685df6612c55a2796be3543928fc25daec007bf9348753104R77

The biggest problem was marking tracked pointers in GC stack slots, and while I somehow found #3277 during my debugging, actually my root cause was much simpler, that stack slot tracking is disabled with -gc=none 😂 The input I got while finding #3277 really helped me to find this though

https://github.com/tinygo-org/tinygo/blob/release/compileopts/config.go#L102

I was able to get my custom GC working with this change to TinyGo

https://github.com/tinygo-org/tinygo/compare/dev...anuraaga:gc-hack?expand=1

Naturally, it seems to make sense to disable stack tracking when gc=none given it's less for custom GC's and more for disabling GC. Would it be reasonable to have a separate gc=custom flag that would enable stack tracking, and also stub out initHeap to allow initialization logic to run in the correct order (I had to work around this once in corazawaf/coraza-proxy-wasm#63 and it's still fragile)?

Or otherwise, perhaps we can remove the guards on gc.conservative enabling stack tracking, i.e. apply my two-line change above? I guess there won't be any tracking if gc=none and no allocations are actually made so it wouldn't hurt to allow the transformation to run in that case too, maybe slightly slower during compile but I'd expect no runtime effect

@deadprogram deadprogram added enhancement New feature or request wasm WebAssembly next-release Will be part of next release labels Nov 12, 2022
@aykevl
Copy link
Member

aykevl commented Nov 17, 2022

Can I propose an alternative? Namely, to integrate bdwgc as an alternative GC in TinyGo? It would be quite a useful tradeoff in size vs speed here: the Boehm GC is heavily optimized for performace (I believe GCC uses it for example) while the TinyGo GC is mostly concerned with speed. In addition, the Boehm GC already supports threads which is one of the major blockers to supporting parallelism on regular "big" OSes (Windows/macOS/Linux).

@anuraaga
Copy link
Contributor Author

Having -gc=bdwgc would be nice, but it will be a lot of effort starting with upstreaming some wasi-related batches to bdwgc, still needing to also see if there is a clean workaround to wasi-libc mmap not supporting the alignment requirement, we have a custom implementation.

Would it be reasonable to add -gc=custom first since it should be relatively straightforward? Even with a future built-in bdwgc, it could at least still be useful for alternative GCs. Otherwise we'll be stuck for a while on the pre-revert commit.

@jrauschenbusch
Copy link

jrauschenbusch commented Feb 8, 2023

@anuraaga When do you plan to start with the implementation of bdwgc? Currently we face the issue that we cannot update the tinygo compiler due to a different memory mgmt than in 0.25. This causes our Go WASM filters to run in OOMs, which was not the case before. Basically it's the same issue as described here.

@anuraaga
Copy link
Contributor Author

anuraaga commented Feb 8, 2023

Hi @jrauschenbusch - I've created a package at https://github.com/wasilibs/nottinygc. Haven't released a tag or even set up CI for it yet as it relies on unreleased TinyGo, but after that's released I'll do that. In the meantime it can still be used for the adventurous

@deadprogram
Copy link
Member

0.27.0 has now been released, so closing. Thank you everyone!

@deadprogram deadprogram removed the next-release Will be part of next release label Feb 12, 2023
@anuraaga
Copy link
Contributor Author

Thanks @deadprogram! @jrauschenbusch I have released nottinygc so feel free to give it a try

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request wasm WebAssembly
Projects
None yet
Development

No branches or pull requests

4 participants