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

docs/guides: additional resources for WebAssembly #352

Merged
merged 1 commit into from
May 17, 2023

Conversation

deadprogram
Copy link
Member

This PR adds more to docs/guides with additional resources for WebAssembly.

Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

Good idea, but I'm not sure about some of the blog posts. If we're going to link to external posts, we should make sure they're at least using current recommendations and not all of them do unfortunately.

Comment on lines 19 to 20
**Shrink Your TinyGo WebAssembly Modules by 60%**
https://www.fermyon.com/blog/optimizing-tinygo-wasm
Copy link
Member

Choose a reason for hiding this comment

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

This one still uses -wasm-abi=generic which has been removed a while ago. Otherwise it seems fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed from this PR, can add to this page once it is updated, I suppose.

Comment on lines +25 to +23
**wazero - TinyGo**
https://wazero.io/languages/tinygo/
Copy link
Member

Choose a reason for hiding this comment

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

This one contains some outdated information on malloc and free. I'm not sure we should encourage people reading this? (Or perhaps send a message to the blog post writers to update?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Or perhaps send a message to the blog post writers to update?

That for sure! cc @codefromthecrypt

Choose a reason for hiding this comment

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

the jury is still out on the malloc/free stuff, but coming in soon. We need to see benchmark before after and it is nearly there.

Choose a reason for hiding this comment

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

basically there are a lot of people using the existing approach (which was reviewed by several) we owe it when doing an update to say what the new is and what it will cost you tetratelabs/wazero#1390

Copy link
Member

Choose a reason for hiding this comment

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

@codefromthecrypt I'm not sure what you mean? malloc and free has been safe for use since tinygo-org/tinygo#3148 and I doubt we'll ever change that back.
A hand rolled malloc will have a much bigger chance of being subtly wrong.

Choose a reason for hiding this comment

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

@aykevl Thanks. We we were missing the context

What you said "malloc and free has been safe for use since tinygo-org/tinygo#3148 and I doubt we'll ever change that back." should be in the PR desc as rationale to use the CGO as the only way and remove the others, especially as the benchmarks don't show any perf regression. The same PR can update the site docs to only say one way (CGO malloc/free). @lburgazzoli can you update tetratelabs/wazero#1390 accordingly? Once that's in, I'll we can remove the manual exports from tinymem, leaving it only used for slice helper functions.

Copy link
Member

Choose a reason for hiding this comment

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

What you said "malloc and free has been safe for use since tinygo-org/tinygo#3148 and I doubt we'll ever change that back." should be in the PR desc as rationale to use the CGO as the only way and remove the others, especially as the benchmarks don't show any perf regression.

@codefromthecrypt I'm not sure I follow. Was this a reply to me, or which PR are you referring to?

Copy link
Member Author

Choose a reason for hiding this comment

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

@lburgazzoli can you update tetratelabs/wazero#1390 accordingly?

I think @codefromthecrypt was asking @lburgazzoli to update tetratelabs/wazero#1390 accordingly. 😸

content/docs/guides/webassembly/resources.md Outdated Show resolved Hide resolved
Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

I didn't fully look at the new blog post series.
I'm also not sure whether it's worth it to review each and everyone of them. Instead, I think it's more important to provide good primary documentation in the first place, which, to be very honest, we have very little of. So, I guess that's a weak LGTM on this PR from me.

Comment on lines +28 to +32
**Wazero, first steps**
https://k33g.hashnode.dev/series/wazero-first-steps
Copy link
Member

Choose a reason for hiding this comment

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

Some things I noticed:

// readBufferFromMemory returns a buffer from WebAssembly
func readBufferFromMemory(bufferPosition *uint32, length uint32) []byte {
    subjectBuffer := make([]byte, length)
    pointer := uintptr(unsafe.Pointer(bufferPosition))
    for i := 0; i < int(length); i++ {
        s := *(*int32)(unsafe.Pointer(pointer + uintptr(i)))
        subjectBuffer[i] = byte(s)
    }
    return subjectBuffer
}

This is one example where unsafe.Slice would be perfect (although I'm not sure about the uint32 vs byte conversion, or what is supposed to be happening there):

// readBufferFromMemory returns a buffer from WebAssembly
func readBufferFromMemory(bufferPosition *byte, wordLength uint32) []byte {
    length := wordLength * 4
    subjectBuffer := make([]byte, length)
    copy(subjectBuffer, unsafe.Slice(bufferPosition, length))
    return subjectBuffer
}

Signed-off-by: deadprogram <ron@hybridgroup.com>
@deadprogram
Copy link
Member Author

Thanks everyone for the feedback. Now merging.

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