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.

Sign up for GitHub

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
Closed

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

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
25146cc
Do not free memory that needs to persist in host
gmlewis Jun 15, 2024
0246b31
Tweak free calls
gmlewis Jun 15, 2024
5892998
Fix typo
gmlewis Jun 16, 2024
56d477a
Address review feedback
gmlewis Jun 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions extism_pdk.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,9 @@ func OutputJSON(v any) error {
return err
}

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.

return nil
}

Expand Down Expand Up @@ -149,6 +151,7 @@ func SetError(err error) {

func SetErrorString(err string) {
mem := AllocateString(err)
defer mem.Free()
extism_error_set(mem.offset)
}

Expand Down Expand Up @@ -190,6 +193,7 @@ func Log(level LogLevel, s string) {

func GetVar(key string) []byte {
mem := AllocateBytes([]byte(key))
defer mem.Free()

offset := extism_var_get(mem.offset)
clength := extism_length(offset)
Expand All @@ -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


extism_var_set(keyMem.offset, valMem.offset)
}

func GetVarInt(key string) int {
mem := AllocateBytes([]byte(key))
defer mem.Free()

offset := extism_var_get(mem.offset)
clength := extism_length(offset)
Expand All @@ -236,13 +240,13 @@ func SetVarInt(key string, value int) {
binary.LittleEndian.PutUint64(bytes, uint64(value))

valMem := AllocateBytes(bytes)
defer valMem.Free()

extism_var_set(keyMem.offset, valMem.offset)
}

func RemoveVar(key string) {
mem := AllocateBytes([]byte(key))
defer mem.Free()
extism_var_set(mem.offset, 0)
}

Expand Down
Loading