GC Warning: Failed to expand heap #234
Comments
|
Looks it it could be related to using "nottinygc" wasilibs/nottinygc#13 |
|
I investigated and the plugin works normaly with the following patch applied (disableing nottinygc) diff --git a/init_tinygo.go b/init_tinygo.go
index 11fb7e9..15dd236 100644
--- a/init_tinygo.go
+++ b/init_tinygo.go
@@ -7,8 +7,7 @@ package main
import (
"unsafe"
-
- _ "github.com/wasilibs/nottinygc"
+ //_ "github.com/wasilibs/nottinygc"
)
// Some host functions that are not implemented by Envoy end up getting imported anyways
diff --git a/magefiles/magefile.go b/magefiles/magefile.go
index 5607cf4..91aa73c 100644
--- a/magefiles/magefile.go
+++ b/magefiles/magefile.go
@@ -188,7 +188,7 @@ func Build() error {
}
buildTags := []string{
- "custommalloc", // https://github.com/wasilibs/nottinygc#usage
+ //"custommalloc", // https://github.com/wasilibs/nottinygc#usage
"no_fs_access", // https://github.com/corazawaf/coraza#build-tags
"memoize_builders", // https://github.com/corazawaf/coraza#build-tags
}
@@ -214,8 +214,8 @@ func Build() error {
initialPages = ip
}
}
-
- if err := sh.RunV("tinygo", "build", "-gc=custom", "-opt=2", "-o", filepath.Join("build", "mainraw.wasm"), "-scheduler=none", "-target=wasi", buildTagArg); err != nil {
+
+ if err := sh.RunV("tinygo", "build", "-opt=2", "-o", filepath.Join("build", "mainraw.wasm"), "-scheduler=none", "-target=wasi", buildTagArg); err != nil {
return err
} |
|
Hi @woehrl01, thanks for raising it.
Would you mind sharing the rough size of the additional files you are including? Are your changes strictly related to adding files with rules (no changes in the build process)? Maybe also giving it a go with this debug branch might print some hints. Thanks to @anuraaga, there has been a significant effort moving to |
|
@M4tteoP Thank you, absolutely! Sizes without the additional file: Sizes with the additional file (so approximately 6MB more) So to be fully transparent about what I'm doing: I'm trying to get geoip based blocking to work in this wasm. Therefore, I created a custom branch from the extended plugin: corazawaf/coraza-geoip@master...woehrl01:coraza-geoip:support_v3_4 And I add the loading of the database as an By purely adding the 6MB database as By switching to the "default" gc, everything works and the blocking also works based on the geoip. |
|
I did some tries only embedding different files and spinning up envoy. It seems to be not directly related to the size of the embedded file but rather to their content 🤔 Total size: Total size: |
|
@M4tteoP Great, that's a very interesting finding, and makes sense because the geopip database is a binary file. |
|
Thanks for the investigation @M4tteoP! Yeah this does confirm a suspicion I have had - random data will cause issues for the GC. bdwgc is by default a conservative GC and relies on finding values in memory that look like pointers to identify what to keep. Random data looks like pointers in 32-bits, which Wasm is (it's funny seeing the Wasm is the next compute hype when we see it taking us back 10 years :P). Generally random data within request parsing is not as big of a deal since the buffers themselves eventually get released and stop causing any false positives, but embedded data is there forever and holding up GC of pointers that look like those values. For reference, bdwgc has this note on 2GB heaps on 32-bit machines, while I don't know if we're running close to that in these cases, we are definitely using a reasonably high amount of memory on a 32-bit machine TinyGo with conservative GC will probably have similar issues but may run into them less by having more knowledge of the way it lays out memory while bdwgc is treating it as a complete black box. If it works for you, it could be a workaround for now, but just for reference, when I try the patch to use default GC and run bdwgc does have some support for precise GC which shouldn't have these issues, but both it and TinyGo's related functionality for it aren't that well documented and I was having trouble using it when experimenting once. I wasn't sure it would have a real benefit at the time so stopped exploring it, but now seeing these cases, I think I'll try again but can't guarantee succeeding anytime soon if at all. |
|
I realized there is no step at all but having the file in the folder since we auto embed it, and |
|
Luckily tinygo does have the docs that I was missing before this time around https://github.com/tinygo-org/tinygo/blob/release/src/runtime/gc_precise.go#L24 I will look at adding more support for precise GC, but for now I made the trivial change to handle just empty objects, which means embedded byte arrays too 😎 Looks like it fixes it and probably improves performance/reduces memory usage in general a lot https://github.com/wasilibs/nottinygc/pull/24/files |
|
Thanks @anuraaga, I was also wondering because at least in proxy-wasm embedding files as variables is common so we probably need give the visibility to the practice of freeing the variable after loading its data. Specifically talking about proxy wasm it should happen after we create the waf. |
|
Hmm - I'm not sure if in practice it matters, embedded data is part of the executable and not part of anything that can be GC'd or released I think. On a normal OS, embedded data is just mmap'd in from the executable, so it will be paged in by the OS when needed, and if not used will be paged out automatically by the OS (not Go). I'm not familiar with how Wasm runtimes deal with the data section in the binary, but I guess it could be possible for them to do something similar, but I suspect more likely the memory's used and not releasable in any way. |
|
Awesome, thanks for the incredible quick fix. The wasm plugin now loads correctly but spills the following errors: @M4tteoP @jcchavezs Would be interested in a PR which adds GeoIP support (using an empty file for the public) which could be easily extended by dropping the database file into the repo for self compilation? |
|
Thanks for the report about the log message, I have released a patch release to suppress the message by default as bdwgc recommends for release builds https://github.com/ivmai/bdwgc/blob/242a3a7b6040c2680e009367e96a77b0e167619a/misc.c#L1961 There is information about the message itself here I believe it shows up because due to some limitations integrating bdwgc with TinyGo, we can't be fully precise. A few messages on startup should generally not be an issue and I think bdwgc recommends suppressing the message to not worry about it if the app performs well enough in practice, and if it doesn't then we'll need to see if there's some way to get the more precise behavior in |
|
@anuraaga awesome, I just confirmed that the error is now gone. 👍🏼 Thank you 🙏🏼 |
I think that would be great! Being something that requires a self-compilation I would make it opt-in, maybe with a dedicated build tag and some documented steps in the readme that could be something like adding your own GeoIP database and enabling the tag. |
Hi there,
I'm receiving the following error:
I already pumped up the istio pods to 4 GB (instead of 1 GB) but still the same error.
Do you have any idea how to troubleshoot that?
Rules are as follows:
I'm running Istio 1.18.*
We build our own plugin because we include additional files. And it looks like that's related to the embedded files being too big. As soon as I remove the files, it works as expected.
The text was updated successfully, but these errors were encountered: