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

examples(allocation): ensures data used in host functions isn't freed before invocation #1434

Closed
wants to merge 1 commit into from

Conversation

lburgazzoli
Copy link
Contributor

No description provided.

@codefromthecrypt codefromthecrypt changed the title examples(allocation): ensure the data use to call host functions is not fred before the host function is invoked examples(allocation): ensures data used in host functions isn't freed before invocation May 4, 2023
@codefromthecrypt
Copy link
Contributor

Thanks! mind doing this and sharing (before/after bench) in the PR description?

# from main
go test -run='^$' -bench '^BenchmarkAllocation.*' ./internal/integration_test/vs/compiler -count=6 > old.txt

# from this commit
go test -run='^$' -bench '^BenchmarkAllocation.*' ./internal/integration_test/vs/compiler -count=6 > new.txt

benchstat old.txt new.txt 

@lburgazzoli
Copy link
Contributor Author

➜ benchstat old.txt new.txt 


goos: linux
goarch: amd64
pkg: github.com/tetratelabs/wazero/internal/integration_test/vs/compiler
cpu: Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz
                          │   old.txt    │              new.txt               │
                          │    sec/op    │    sec/op     vs base              │
Allocation/Compile-12       15.63m ± 61%   15.57m ± 82%       ~ (p=0.937 n=6)
Allocation/Instantiate-12   353.2µ ± 17%   357.4µ ±  5%       ~ (p=0.937 n=6)
Allocation/Call-12          3.212µ ±  2%   2.898µ ±  2%  -9.78% (p=0.002 n=6)
geomean                     260.8µ         252.6µ        -3.11%

                          │   old.txt    │               new.txt                │
                          │     B/op     │     B/op      vs base                │
Allocation/Compile-12       2.817Mi ± 0%   2.816Mi ± 0%       ~ (p=0.065 n=6)
Allocation/Instantiate-12   351.4Ki ± 0%   351.4Ki ± 0%       ~ (p=0.818 n=6)
Allocation/Call-12            48.00 ± 0%     48.00 ± 0%       ~ (p=1.000 n=6) ¹
geomean                     36.22Ki        36.21Ki       -0.01%
¹ all samples are equal

                          │   old.txt   │               new.txt               │
                          │  allocs/op  │  allocs/op   vs base                │
Allocation/Compile-12       2.022k ± 0%   2.012k ± 0%  -0.49% (p=0.002 n=6)
Allocation/Instantiate-12    852.0 ± 0%    852.0 ± 0%       ~ (p=1.000 n=6) ¹
Allocation/Call-12           5.000 ± 0%    5.000 ± 0%       ~ (p=1.000 n=6) ¹
geomean                      205.0         204.7       -0.17%
¹ all samples are equal

@lburgazzoli lburgazzoli marked this pull request as ready for review May 4, 2023 08:04
Copy link
Contributor

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

code wise I like it.

While personally, I rarely understand when I'm supposed to do runtime.KeepAlive, this seems the pattern to do it. What I like also is that the stringToPtr code is more concise.

One homework before merge is checking that unsafe.StringData won't return nil when compiled with TinyGo's wasi target.

examples/allocation/tinygo/testdata/greet.go Show resolved Hide resolved
examples/allocation/tinygo/testdata/greet.go Show resolved Hide resolved
examples/allocation/tinygo/testdata/greet.go Outdated Show resolved Hide resolved
@ncruces
Copy link
Collaborator

ncruces commented May 4, 2023

While we're at it, is there a reason not to do the below change:

func ptrToString(ptr uint32, size uint32) string {
return unsafe.String((*byte)(unsafe.Pointer(uintptr(ptr))), size)
}

Seems to match guidance from tinygo-org/tinygo#1284

@lburgazzoli lburgazzoli force-pushed the keepalive branch 2 times, most recently from 6bd4122 to c1f1bd5 Compare May 4, 2023 12:29
@codefromthecrypt
Copy link
Contributor

Sorry I wasn't sure which line we were proceeding with, so I commented on #1436 (review) instead of this one. Feel free to proceed forward with whatever, as I've already bookmarked 1436 for next time

@ncruces
Copy link
Collaborator

ncruces commented May 5, 2023

I think we should merge this one. I did my changes offline and didn't notice this PR before push.

@lburgazzoli
Copy link
Contributor Author

No objection merge either this or #1436 , they are essentially equivalent

@evacchi
Copy link
Contributor

evacchi commented May 5, 2023

merged #1436 thanks @lburgazzoli

@evacchi evacchi closed this May 5, 2023
@lburgazzoli lburgazzoli deleted the keepalive branch May 5, 2023 09:22
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.

4 participants