Add -gc=custom option #3302

anuraaga · 2022-11-21T02:35:31Z

A custom GC can be useful for prototyping GC algorithms or writing app-specific GCs that optimize based on knowledge local to an app (e.g., scoped arenas). There is interest in adding bdwgc to TinyGo, but in preparation I think this option can make prototyping that easier, as well as any future improvements.

anuraaga · 2022-11-21T02:36:41Z

src/runtime/gc_stack_portable.go

@@ -61,3 +55,12 @@ func trackPointer(ptr unsafe.Pointer)
 func swapStackChain(dst **stackChainObject) {
 	*dst, stackChainStart = stackChainStart, *dst
 }
+
+func init() {


Since custom GC won't call markStack it gets removed during compilation as does the volatility. An init method is never removed so seems to allow the volatility to be correct regardless of markStack.

I agree at the moment it's not removed, I'm just concerned about smarter optimizers that notice that all the writes to stackChainStart happen after the load..

Maybe we should keep the volatile load in markStack and add one in init as you have here?

(Edit: tested this with the standard conservative gc and it seems to work.)

That's a good point. I went ahead and added it to both locations for now

Don't put it in an init. As I said in the comment above, the following is unsafe:

Since custom GC won't call markStack

Therefore, because markStack must be used, this change can be reverted.

anuraaga · 2022-11-21T02:37:24Z

src/runtime/gc_custom.go

+}
+
+func markRoots(start, end uintptr) {
+	// GC manages roots so ignore to allow gc_stack_portable to compile


Some shuffling of code into separate files with build tags could remove the need for these stubs but it didn't seem worth it

This is unsafe. The GC must call markStack which will in turn call markRoots. Otherwise the stack won't be scanned for objects. The reason this call is ignored in -gc=leaking and -gc=none is because they don't deallocate anything anyway.

The custom GC can access the stack pointer and mark the stack without calling this function itself. In the case of the bdwgc implementation, it's C code to read the stack via a pointer to a local variable seems to work fine, and if wasn't our code would crash quickly as for other cases of missing the stack.

More abstractly, when using a library like bdwgc there is no API to mark - just add roots (not needed for the stack as above though). So there isn't really a way to wire this to it.

In the case of the bdwgc implementation, it's C code to read the stack via a pointer to a local variable

I don't think this is correct for WebAssembly. Yes, this works for any other architecture but there is a reason why we need to keep this custom shadow stack in TinyGo for WebAssembly: WebAssembly can put values on the VM evaluation stack which isn't accessible from outside that particular function.
If bdwgc somehow manages to read the stack without this, please let me know how it is able to do this. Because I'm pretty sure this is fundamentally impossible.

seems to work fine, and if wasn't our code would crash quickly as for other cases of missing the stack.

No. I've found GC bugs where some roots weren't marked after months of use, so "it doesn't crash" isn't enough proof it works fine.

The shadow stack is managed by copying to the C stack right? That's why it works, since the C stack is marked.

We see that thanks to recent changes, that's all markStack does in TinyGo too

https://github.com/tinygo-org/tinygo/blob/release/src/runtime/gc_stack_raw.go#L19

It is just marking the C stack. That's what bdwgc does too and any custom GC would be doing it. There doesn't seem to be a need for an explicit call to do it if the C library implementing GC does it automatically

As far as I can tell the main concern is still the volatile load - it's not so much that markStack is needed for finding the stack but that it is needed to guarantee the shadow stack is fresh. Implementations can choose to ignore the call to markRoots if they don't need it, as with bdwgc.

I went ahead and reverted the volatile load change and added markRoots here, which with bdwgc will be no-op. Hopefully that makes sense

Hmm, you are right. It is indeed enough to force the compiler to have an up-to-date shadow stack by doing the volatile load. Then it can scan the globals conservatively and find the current stack that way.

anuraaga · 2022-11-21T02:37:51Z

src/runtime/arch_tinygowasm_malloc.go

@@ -1,5 +1,5 @@
-//go:build tinygo.wasm && !custommalloc


custommalloc was added recently and not released yet, if adding gc.custom it doesn't seem worth having a separate tag for it.

anuraaga · 2022-11-21T02:38:39Z

Confirmed the interface works in https://github.com/corazawaf/coraza-proxy-wasm/compare/main...anuraaga:gccustom?expand=1

dgryski · 2022-11-22T18:00:57Z

@aykevl Are you happy with this?

jcchavezs · 2022-11-29T23:11:05Z

Any movement @aykevl?

aykevl · 2022-12-01T17:35:39Z

@jcchavezs I'm currently relocating so unfortunately I don't really have the time to look at this right now. Hopefully next week.

aykevl

Did a first pass review. It looks quite reasonable, with a few notes here and there on how to do things more safely.

That said, I don't think we will want to promise any kind of API stability here, and I think this should be noted in gc_custom.go. The reason is that promising anything like that will make this feature a much bigger maintenance burden for a feature that shouldn't be used much anyway (if it is used a lot, support for that GC should be implemented upstream instead).

compileopts/config.go

aykevl · 2022-12-12T19:19:09Z

src/runtime/gc_custom.go

+// - func KeepAlive(x interface{})
+// - func SetFinalizer(obj interface{}, finalizer interface{})


See #3336. We don't support these yet so I've simplified things a bit for now.

It's not a high priority but I was going to implement setFinalizer in the custom implementation. Is it ok to keep them to try it? I guess in principle anyways a finalizer is a feature of the gc

I'm interested in implementing finalizer, but for now I've removed the methods from this PR to make sure it compiles with the dev branch where the change was merged

aykevl · 2022-12-12T19:22:10Z

src/runtime/gc_custom.go

+// In addition, if targeting wasi, the following functions should be exported for interoperability
+// with wasi libraries that use them. Note, this requires the export directive, not go:linkname.
+//
+// - func malloc(size uintptr) unsafe.Pointer
+// - func free(ptr unsafe.Pointer)
+// - func calloc(nmemb, size uintptr) unsafe.Pointer
+// - func realloc(oldPtr unsafe.Pointer, size uintptr) unsafe.Pointer


These are normally defined by the runtime, and call runtime.alloc/runtime.free as needed. Would it be possible to keep it that way and let the custom GC only provide the functions above?

We added the custommalloc build tag and this intends to replace it with just the concept of a custom GC.

Replacing malloc with an implementation that doesn't delegate to Go GC (in my case mimalloc) had a dramatic improvement in performance and is important for allowing high performance. I'd be fine with keeping the current tag though if it seems better.

aykevl · 2022-12-12T19:26:26Z

src/runtime/gc_custom.go

+}
+
+func markRoots(start, end uintptr) {
+	// GC manages roots so ignore to allow gc_stack_portable to compile


This is unsafe. The GC must call markStack which will in turn call markRoots. Otherwise the stack won't be scanned for objects. The reason this call is ignored in -gc=leaking and -gc=none is because they don't deallocate anything anyway.

aykevl · 2022-12-12T19:30:02Z

src/runtime/gc_stack_portable.go

@@ -61,3 +55,12 @@ func trackPointer(ptr unsafe.Pointer)
 func swapStackChain(dst **stackChainObject) {
 	*dst, stackChainStart = stackChainStart, *dst
 }
+
+func init() {


Don't put it in an init. As I said in the comment above, the following is unsafe:

Since custom GC won't call markStack

Therefore, because markStack must be used, this change can be reverted.

anuraaga · 2022-12-13T01:39:09Z

BTW this is the implementation of the GC with this PR in case missed from the linked issue

https://github.com/corazawaf/coraza-proxy-wasm/blob/main/internal/gc/gc_conservative.go

I'm fairly confident that the stack is getting marked fine without any explicit call to anything because before #3291, it was quickly crashing and after that it's stable

Co-authored-by: Ayke <[email protected]>

anuraaga · 2023-01-12T00:04:01Z

Thanks for the comment @aykevl - is there anything left for this PR? Think I addressed all the comments. Thanks!

anuraaga · 2023-01-20T06:22:34Z

Rebased once more, probably the last time. If this isn't going in, really need a response saying that so we can move on

jcchavezs · 2023-01-27T22:50:01Z

Ping @aykevl

aykevl

This looks good to me. It doesn't touch many parts of the runtime and therefore should have a low maintenance overhead.

@dgryski any comments before I merge?

dgryski · 2023-01-28T16:18:48Z

Fine to merge from me. It might be nice to have a unit test that checks we don't break this in the future, but that's not a blocking issue for me.

aykevl · 2023-01-28T16:41:23Z

Yeah a test would be nice, but that means we'd have to at least make a stub custom allocator.
As long as we don't advertise this as a supported feature (but rather "use at your own risk") I'm fine without a test.

dgryski · 2023-01-28T19:19:28Z

Ok, so let's merge and add a doc+warning in another PR.

deadprogram · 2023-01-28T19:23:16Z

Now going to squash/merge.

Thank you very much @anuraaga for all your work and patience on this PR. Also thanks very much @aykevl and @dgryski for such careful review.

* Add -gc=custom option

Add -gc=custom option

b3d1d87

anuraaga commented Nov 21, 2022

View reviewed changes

anuraaga added 2 commits November 21, 2022 11:57

Drift

74aed5e

Load stack in init and markStack

79f62b2

Merge branch 'dev' of github.com:tinygo-org/tinygo into custom-gc

e27e61e

aykevl mentioned this pull request Dec 12, 2022

runtime: move KeepAlive/SetFinalizer to common code #3336

Merged

aykevl reviewed Dec 12, 2022

View reviewed changes

anuraaga and others added 8 commits December 13, 2022 10:39

Update compileopts/config.go

d0124d7

Co-authored-by: Ayke <[email protected]>

Revert loading stack change and require addRoots

d9b9d94

Merge branch 'custom-gc' of github.com:anuraaga/tinygo into custom-gc

0fcce65

Revert newline

63b2944

Merge branch 'dev' of github.com:tinygo-org/tinygo into custom-gc

9f57f63

Revert unintended mingw update

36a73df

Remove methods that were moved

f58fe76

Document runtime.markStack

857e613

anuraaga mentioned this pull request Dec 21, 2022

Print attempted allocation size when panicing OOM #3236

Closed

Merge branch 'dev' of github.com:tinygo-org/tinygo into custom-gc

ca895bf

This was referenced Jan 5, 2023

Memory leak when using GetHttpRequestHeaders tetratelabs/proxy-wasm-go-sdk#349

Closed

map delete not finding keys when value is largeish, key hash seems to be incorrect #3358

Closed

Merge branch 'dev' of github.com:tinygo-org/tinygo into custom-gc

75e0fc1

aykevl approved these changes Jan 28, 2023

View reviewed changes

deadprogram merged commit eebd2f6 into tinygo-org:dev Jan 28, 2023

deadprogram pushed a commit that referenced this pull request Feb 14, 2023

Add -gc=custom option (#3302)

0ec095e

* Add -gc=custom option

LiuJiazheng pushed a commit to LiuJiazheng/tinygo that referenced this pull request Aug 20, 2023

Add -gc=custom option (tinygo-org#3302)

92639bf

* Add -gc=custom option

		// - func KeepAlive(x interface{})
		// - func SetFinalizer(obj interface{}, finalizer interface{})

Add -gc=custom option #3302

Add -gc=custom option #3302

Conversation

anuraaga commented Nov 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga Dec 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga commented Nov 21, 2022

dgryski commented Nov 22, 2022

jcchavezs commented Nov 29, 2022

aykevl commented Dec 1, 2022

aykevl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga commented Dec 13, 2022

anuraaga commented Jan 12, 2023

anuraaga commented Jan 20, 2023

jcchavezs commented Jan 27, 2023

aykevl left a comment

Choose a reason for hiding this comment

dgryski commented Jan 28, 2023

aykevl commented Jan 28, 2023

dgryski commented Jan 28, 2023

deadprogram commented Jan 28, 2023

anuraaga Dec 16, 2022 •

edited

Loading