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

Port to WASM #27

Open
wants to merge 15 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@nightexcessive
Member

nightexcessive commented Sep 16, 2018

This is a port to WASM. I'm not sure that, given that this is a gopherjs library, we should officially adopt WASM as the only way of using the library.

Making a library that works in both WASM and GopherJS is a much larger scope than I wanted to take on. There are differences in execution environment and callbacks that make them behave significantly differently.

I've also changed how tests work, given that we can use go test for WASM testing. That is documented in HOW_TO_TEST.md.

This passes all of the current tests.

Resolves #25.

@nightexcessive nightexcessive requested a review from dmitshur Sep 16, 2018

@hajimehoshi

This comment has been minimized.

Show comment
Hide comment
@hajimehoshi

hajimehoshi Sep 16, 2018

Member

I was wondering if this is different from #26 .

Member

hajimehoshi commented Sep 16, 2018

I was wondering if this is different from #26 .

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 16, 2018

Member

This is great, thank you for working on this @nightexcessive! I tested this quickly with one of my projects that uses WebSockets substantially, and everything was working as expected when compiled with WebAssembly.

We certainly can't merge this as is and remove the GopherJS support at the same import path, as that would break all existing users. However, I think we can and should support both.

Making a library that works in both WASM and GopherJS is a much larger scope than I wanted to take on. There are differences in execution environment and callbacks that make them behave significantly differently.

I don't quite understand why it'd be difficult. I'll limit discussion to the high-level bindings package only. We currently have the following API at github.com/gopherjs/websocket:

// Dial opens a new WebSocket connection. It will block until the connection is
// established or fails to connect.
func Dial(url string) (net.Conn, error)

I think we need to follow this Go proverb:

Syscall must always be guarded with build tags.

That applies to syscall/js and github.com/gopherjs/gopherjs/js packages, they're basically the equivalent of syscall for the browser.

We can have two implementations of the Dial function in two separate files, e.g.:

  • conn_js.go:

    // +build js,!wasm
    
    package websocket
    
    import (
    	// ...
    	"github.com/gopherjs/gopherjs/js"
    	"github.com/gopherjs/websocket/websocketjs"
    )
    
    func Dial(url string) (net.Conn, error) {
    	// ... GopherJS implementation
    	// ... use websocketjs package as before
    }
  • conn_wasm.go:

    // +build js,wasm
    
    package websocket
    
    import (
    	// ...
    	"syscall/js"
    )
    
    func Dial(url string) (net.Conn, error) {
    	// ... WebAssembly implementation
    	// ... don't import websocketjs, just copy and directly integrate
    	//     its code, modified for WebAssembly
    }

(Optionally, we can have the package level Dial call a build tag-constrained dial implementation. That way, it's easier to enforce and document the public API in one place.)

Is this an approach you've considered @nightexcessive?

Edit: To clarify, I'm only suggesting we add WebAssembly support to websocket package, not the low-level websocketjs binding. We can/should copy the websocketjs code modified for WebAssembly into websocket as unexported code.

Member

dmitshur commented Sep 16, 2018

This is great, thank you for working on this @nightexcessive! I tested this quickly with one of my projects that uses WebSockets substantially, and everything was working as expected when compiled with WebAssembly.

We certainly can't merge this as is and remove the GopherJS support at the same import path, as that would break all existing users. However, I think we can and should support both.

Making a library that works in both WASM and GopherJS is a much larger scope than I wanted to take on. There are differences in execution environment and callbacks that make them behave significantly differently.

I don't quite understand why it'd be difficult. I'll limit discussion to the high-level bindings package only. We currently have the following API at github.com/gopherjs/websocket:

// Dial opens a new WebSocket connection. It will block until the connection is
// established or fails to connect.
func Dial(url string) (net.Conn, error)

I think we need to follow this Go proverb:

Syscall must always be guarded with build tags.

That applies to syscall/js and github.com/gopherjs/gopherjs/js packages, they're basically the equivalent of syscall for the browser.

We can have two implementations of the Dial function in two separate files, e.g.:

  • conn_js.go:

    // +build js,!wasm
    
    package websocket
    
    import (
    	// ...
    	"github.com/gopherjs/gopherjs/js"
    	"github.com/gopherjs/websocket/websocketjs"
    )
    
    func Dial(url string) (net.Conn, error) {
    	// ... GopherJS implementation
    	// ... use websocketjs package as before
    }
  • conn_wasm.go:

    // +build js,wasm
    
    package websocket
    
    import (
    	// ...
    	"syscall/js"
    )
    
    func Dial(url string) (net.Conn, error) {
    	// ... WebAssembly implementation
    	// ... don't import websocketjs, just copy and directly integrate
    	//     its code, modified for WebAssembly
    }

(Optionally, we can have the package level Dial call a build tag-constrained dial implementation. That way, it's easier to enforce and document the public API in one place.)

Is this an approach you've considered @nightexcessive?

Edit: To clarify, I'm only suggesting we add WebAssembly support to websocket package, not the low-level websocketjs binding. We can/should copy the websocketjs code modified for WebAssembly into websocket as unexported code.

@nightexcessive

This comment has been minimized.

Show comment
Hide comment
@nightexcessive

nightexcessive Sep 16, 2018

Member

I haven't considered that approach. I was considering the gopherwasm/js package only. I think I've modified it such that it may work with both gopherjs and wasm using gopherwasm/js. I don't know how to compile the tests to work with gopherjs, however.

Member

nightexcessive commented Sep 16, 2018

I haven't considered that approach. I was considering the gopherwasm/js package only. I think I've modified it such that it may work with both gopherjs and wasm using gopherwasm/js. I don't know how to compile the tests to work with gopherjs, however.

@nightexcessive

This comment has been minimized.

Show comment
Hide comment
@nightexcessive

nightexcessive Oct 6, 2018

Member

I've made it use the gopherwasm/js package instead of syscall/js. However, GopherJS can't seem to compile it when using Go 1.11 on Windows.

Member

nightexcessive commented Oct 6, 2018

I've made it use the gopherwasm/js package instead of syscall/js. However, GopherJS can't seem to compile it when using Go 1.11 on Windows.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 6, 2018

Member

I haven't considered that approach. I was considering the gopherwasm/js package only.

Can we discuss that approach?

My understanding is that using gopherwasm/js is a good idea in the following situations:

  • To take a WebAssembly-only package that uses syscall/js and make it quickly support GopherJS as well.
  • To reduce maintenance costs of a package, since it allows not having two implementation copies (one using syscall/js, another using github.com/gopherjs/gopherjs/js).

However, I think it would be better for us to choose to use syscall/js directly in this package, and maintain two implementation copies (the previous GopherJS one that uses github.com/gopherjs/gopherjs/js, and the new WebAssembly port that uses syscall/js directly).

Here's why I think it's better in this context:

  • It gives us more direct control of the underlying syscall package, and all of its features, even if some happen not to be compatible with GopherJS. This is helpful for performance, and ability to make WebAssembly-specific changes. It does mean more code and more work to maintain it, but I don't think the cost is that high and is worth it.
  • I just think it's better to keep this package as low-overhead as possible, and avoiding an extra abstraction layer is in line with that goal.

That said, using gopherwasm/js is also viable and not a bad choice. If you have arguments for why you'd prefer to go with gopherwasm/js approach, I'm happy to hear them.

What do you think?

Member

dmitshur commented Oct 6, 2018

I haven't considered that approach. I was considering the gopherwasm/js package only.

Can we discuss that approach?

My understanding is that using gopherwasm/js is a good idea in the following situations:

  • To take a WebAssembly-only package that uses syscall/js and make it quickly support GopherJS as well.
  • To reduce maintenance costs of a package, since it allows not having two implementation copies (one using syscall/js, another using github.com/gopherjs/gopherjs/js).

However, I think it would be better for us to choose to use syscall/js directly in this package, and maintain two implementation copies (the previous GopherJS one that uses github.com/gopherjs/gopherjs/js, and the new WebAssembly port that uses syscall/js directly).

Here's why I think it's better in this context:

  • It gives us more direct control of the underlying syscall package, and all of its features, even if some happen not to be compatible with GopherJS. This is helpful for performance, and ability to make WebAssembly-specific changes. It does mean more code and more work to maintain it, but I don't think the cost is that high and is worth it.
  • I just think it's better to keep this package as low-overhead as possible, and avoiding an extra abstraction layer is in line with that goal.

That said, using gopherwasm/js is also viable and not a bad choice. If you have arguments for why you'd prefer to go with gopherwasm/js approach, I'm happy to hear them.

What do you think?

@nightexcessive

This comment has been minimized.

Show comment
Hide comment
@nightexcessive

nightexcessive Oct 7, 2018

Member

Since almost all of the functions in both the websocket and websocketjs package interact with JavaScript, we'd have two choices:

  • We could break out the interaction with JavaScript into its own internal functions, e.g.: websocket.conn.RemoteAddr calls websocket.conn.url to get the connection URL instead of making the JavaScript call directly
  • We would have two complete duplicates to maintain that may get out of sync with enhancements, bug fixes, etc.

Obviously, the former seems more desirable. Regardless of the way we do it, there will almost certainly be an extra layer of abstraction.

Member

nightexcessive commented Oct 7, 2018

Since almost all of the functions in both the websocket and websocketjs package interact with JavaScript, we'd have two choices:

  • We could break out the interaction with JavaScript into its own internal functions, e.g.: websocket.conn.RemoteAddr calls websocket.conn.url to get the connection URL instead of making the JavaScript call directly
  • We would have two complete duplicates to maintain that may get out of sync with enhancements, bug fixes, etc.

Obviously, the former seems more desirable. Regardless of the way we do it, there will almost certainly be an extra layer of abstraction.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 7, 2018

Member

Obviously, the former seems more desirable.

Ok, it sounds like you're feeling pretty strongly about avoiding having two implementations of the high-level websocket package (one using syscall/js, the other using github.com/gopherjs/gopherjs/js) and prefer to have one (using github.com/gopherjs/gopherwasm/js). I'm okay to proceed with that. If there's ever a real need in the future, we can always migrate to the two-implementation approach, since it's just an internal implementation detail (no public API is going to be affected).

Regardless of the way we do it, there will almost certainly be an extra layer of abstraction.

Not sure what extra layer of abstraction you're referring to when syscall/js and github.com/gopherjs/gopherjs/js are used directly. Can you clarify?

However, let's talk about the package organization first.

We have two packages here right now:

I will argue that we have a hard constraint that we cannot break the API of github.com/gopherjs/websocket/websocketjs for GopherJS users. This package has existed for a while, and it's inside the gopherjs organization. I don't think it's viable to break its API and all its existing users this late.

That leaves us with these options of what to do about github.com/gopherjs/websocket/websocketjs for WebAssembly:

  1. We implement WebAssembly support for github.com/gopherjs/websocket/websocketjs with a different API. That means the github.com/gopherjs/websocket/websocketjs package will have different API depending on whether you're trying to compile it with GopherJS compiler or for WebAssembly.
  2. We create a new package at a different import path with a similar API that is the same for GopherJS and WebAssembly uses.
  3. We don't try to add WebAssembly support to github.com/gopherjs/websocket/websocketjs at all.

Option 1 is not great because the same package would have to be used differently depending on which compiler you're trying to use. It feels confusing and hostile (e.g., godoc.org can't even display documentation for different GOOS/GOARCH values).

I think we should pick option 3, and not try to add WebAssembly support for github.com/gopherjs/websocket/websocketjs at all. This low-level package was initially created before the high-level one existed, and it was helpful for people who were more familiar with JavaScript and its WebSocket API rather than Go and its net.Conn interface. By now, people trying to use WebSockets within WebAssembly should be quite comfortable using net.Conn, which is a much more pleasant and powerful experience.

So my suggestion is we leave github.com/gopherjs/websocket/websocketjs alone, and only add WebAssembly support to the high-level github.com/gopherjs/websocket package, where it's possible to do so without changing its API.

Does that make sense, and what do you think? Let me know if anything's confusing.

Member

dmitshur commented Oct 7, 2018

Obviously, the former seems more desirable.

Ok, it sounds like you're feeling pretty strongly about avoiding having two implementations of the high-level websocket package (one using syscall/js, the other using github.com/gopherjs/gopherjs/js) and prefer to have one (using github.com/gopherjs/gopherwasm/js). I'm okay to proceed with that. If there's ever a real need in the future, we can always migrate to the two-implementation approach, since it's just an internal implementation detail (no public API is going to be affected).

Regardless of the way we do it, there will almost certainly be an extra layer of abstraction.

Not sure what extra layer of abstraction you're referring to when syscall/js and github.com/gopherjs/gopherjs/js are used directly. Can you clarify?

However, let's talk about the package organization first.

We have two packages here right now:

I will argue that we have a hard constraint that we cannot break the API of github.com/gopherjs/websocket/websocketjs for GopherJS users. This package has existed for a while, and it's inside the gopherjs organization. I don't think it's viable to break its API and all its existing users this late.

That leaves us with these options of what to do about github.com/gopherjs/websocket/websocketjs for WebAssembly:

  1. We implement WebAssembly support for github.com/gopherjs/websocket/websocketjs with a different API. That means the github.com/gopherjs/websocket/websocketjs package will have different API depending on whether you're trying to compile it with GopherJS compiler or for WebAssembly.
  2. We create a new package at a different import path with a similar API that is the same for GopherJS and WebAssembly uses.
  3. We don't try to add WebAssembly support to github.com/gopherjs/websocket/websocketjs at all.

Option 1 is not great because the same package would have to be used differently depending on which compiler you're trying to use. It feels confusing and hostile (e.g., godoc.org can't even display documentation for different GOOS/GOARCH values).

I think we should pick option 3, and not try to add WebAssembly support for github.com/gopherjs/websocket/websocketjs at all. This low-level package was initially created before the high-level one existed, and it was helpful for people who were more familiar with JavaScript and its WebSocket API rather than Go and its net.Conn interface. By now, people trying to use WebSockets within WebAssembly should be quite comfortable using net.Conn, which is a much more pleasant and powerful experience.

So my suggestion is we leave github.com/gopherjs/websocket/websocketjs alone, and only add WebAssembly support to the high-level github.com/gopherjs/websocket package, where it's possible to do so without changing its API.

Does that make sense, and what do you think? Let me know if anything's confusing.

@hajimehoshi

This comment has been minimized.

Show comment
Hide comment
@hajimehoshi

hajimehoshi Oct 7, 2018

Member

@dmitshur

I don't have a strong opinion on that, but let me comment...

It gives us more direct control of the underlying syscall package, and all of its features, even if some happen not to be compatible with GopherJS. This is helpful for performance, and ability to make WebAssembly-specific changes. It does mean more code and more work to maintain it, but I don't think the cost is that high and is worth it.

Interesting. Would you give me some examples (sorry if I missed)?

I just think it's better to keep this package as low-overhead as possible, and avoiding an extra abstraction layer is in line with that goal.

I tend not to think the overhead is so big that we need to consider, since GopherJS itself's overhead is much bigger.

Member

hajimehoshi commented Oct 7, 2018

@dmitshur

I don't have a strong opinion on that, but let me comment...

It gives us more direct control of the underlying syscall package, and all of its features, even if some happen not to be compatible with GopherJS. This is helpful for performance, and ability to make WebAssembly-specific changes. It does mean more code and more work to maintain it, but I don't think the cost is that high and is worth it.

Interesting. Would you give me some examples (sorry if I missed)?

I just think it's better to keep this package as low-overhead as possible, and avoiding an extra abstraction layer is in line with that goal.

I tend not to think the overhead is so big that we need to consider, since GopherJS itself's overhead is much bigger.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment