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

Do not free memory that needs to persist in the host #34

Closed
wants to merge 4 commits into from

Conversation

gmlewis
Copy link
Contributor

@gmlewis gmlewis commented Jun 15, 2024

As documented in the #js-sdk Discord channel, I believe that the Go PDK should not be freeing memory that needs to persist in the host.
This PR addresses this issue.

@gmlewis
Copy link
Contributor Author

gmlewis commented Jun 15, 2024

I'm now wondering if OutputJSON should have a call to Free() for its AllocateBytes().
Also, RemoveVar should probably free its AllocateBytes.

In fact, I'm now thinking that the Memory objects created in the SetVar and SetVarInt might need to be retained and exposed to the PDK user so that they can then later Free them when necessary.

Additionally, I'm thinking that the MoonBit PDK also needs to do similar things... basically giving more memory control to the PDK user.

Signed-off-by: Glenn Lewis <[email protected]>
@bhelx bhelx requested review from zshipko and nilslice June 15, 2024 15:36
extism_pdk.go Outdated
@@ -230,19 +234,19 @@ func GetVarInt(key string) int {

func SetVarInt(key string, value int) {
keyMem := AllocateBytes([]byte(key))
defer keyMem.Free()
defer mem.Free()
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps I'm missing its definition, but where does mem come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yikes, I should not have done that in a hurry. I'm on a flight now but will fix this later tonight.
Sorry.

OutputMemory(AllocateBytes(b))
mem := AllocateBytes(b)
defer mem.Free()
OutputMemory(mem)
Copy link
Member

Choose a reason for hiding this comment

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

If we'd retain memory that needs to be accessed by the host elsewhere, shouldn't that also be true here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So my thought here was that the host would output the information immediately and the memory could be freed. That seemed to work in the MoonBit PDK but this is one reason why I think a suite of conformance tests would be fantastic... To help answer questions like this one.

Signed-off-by: Glenn Lewis <[email protected]>
@gmlewis
Copy link
Contributor Author

gmlewis commented Jun 16, 2024

I've fixed the error and run the tests, and it all appears to work now. Sorry about that.

$ make example
tinygo build -o example/tiny_countvowels.wasm -target wasi ./example/countvowels
tinygo build -o example/tiny_http.wasm        -target wasi ./example/http
tinygo build -o example/tiny_reactor.wasm     -target wasi ./example/reactor
GOOS=wasip1 GOARCH=wasm go build -tags std -o example/std_countvowels.wasm ./example/countvowels
GOOS=wasip1 GOARCH=wasm go build -tags std -o example/std_http.wasm        ./example/http

$ make test
extism call example/tiny_countvowels.wasm count_vowels --wasi --input "this is a test" --set-config '{"thing": "1234"}'
{"count": 4, "config": "1234", "a": "this is var a"}extism call example/tiny_http.wasm        http_get     --wasi --log-level info --allow-host "jsonplaceholder.typicode.com"
{
  "userId": 1,
  "id": 1,
  "title": "delectus aut autem",
  "completed": false
}

extism call example/tiny_reactor.wasm read_file --input "example/reactor/test.txt" --allow-path ./example/reactor --wasi --log-level info
Hello World!

extism call example/std_countvowels.wasm _start     --wasi --input "this is a test" --set-config '{"thing": "1234"}'
{"count":42,"total":21000000,"vowels":"aAeEiIoOuUyY"}

extism call example/std_http.wasm        _start     --wasi --log-level info --allow-host "jsonplaceholder.typicode.com"
{
  "userId": 1,
  "id": 1,
  "title": "delectus aut autem",
  "completed": false
}

@gmlewis
Copy link
Contributor Author

gmlewis commented Jun 17, 2024

Hmmm... that "total":21000000 looks really suspicious... Investigating.
EDIT: never mind. That value is directly in the example code.

@zshipko
Copy link
Contributor

zshipko commented Jun 17, 2024

It seems like this is actually related to how the js-sdk has implemented var storage, I just opened a PR here: extism/js-sdk#71

Some of these other changes seem good though so I think this could still be worth merging with some small modifications

extism_pdk.go Outdated
@@ -208,13 +212,13 @@ func SetVar(key string, value []byte) {
defer keyMem.Free()

valMem := AllocateBytes(value)
defer valMem.Free()
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add this back since extism/js-sdk#71 addresses the root of the issue

Signed-off-by: Glenn Lewis <[email protected]>
@zshipko zshipko closed this Jun 17, 2024
@gmlewis gmlewis deleted the free-bugs branch June 17, 2024 21:03
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.

3 participants