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

fix: avoid importing net/http on wasm #449

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

paralin
Copy link
Contributor

@paralin paralin commented May 4, 2024

Using goweight: https://github.com/paralin/goweight

GOOS=js GOARCH=wasm goweight | less

Without this change:

7.9 MB runtime
6.6 MB net/http
3.1 MB net
3.0 MB crypto/tls
2.0 MB reflect
1.4 MB math/big
1.3 MB crypto/x509
935 kB os
...

With this change:

7.9 MB runtime
3.1 MB net
2.0 MB reflect
935 kB os
...

Slight modifications to avoid importing net/http reduce the binary size significantly.

@nhooyr nhooyr changed the base branch from master to dev May 5, 2024 01:35
Copy link
Contributor

@nhooyr nhooyr left a comment

Choose a reason for hiding this comment

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

Hi, I understand the reason for this change but unfortunately I cannot merge it as it breaks the API. The API must be kept 1:1 between Go and Wasm and so Dial must return http.Response.

@paralin
Copy link
Contributor Author

paralin commented May 5, 2024

Dial is a global function and the value returned on wasm is a dummy value.

Are people using these in interfaces? is that why? Other than that it seems like anyone using Dial would already be ignoring the dummy value on wasm.

@nhooyr
Copy link
Contributor

nhooyr commented May 5, 2024

People could be wrapping this library in some kind of API/helper for their own code and they might be checking resp or passing it up the chain. This patch would break that.

I don't know if it's common but it doesn't seem far fetched to me. I don't have a better suggestion unfortunately. I'll leave this open for now, let's see if anyone else has thoughts.

@mafredri mafredri deleted the branch coder:master August 15, 2024 20:35
@mafredri mafredri closed this Aug 15, 2024
@mafredri mafredri reopened this Aug 16, 2024
@mafredri mafredri changed the base branch from dev to master August 16, 2024 14:41
Using goweight: https://github.com/paralin/goweight

GOOS=js GOARCH=wasm goweight | less

Without this change:

  7.9 MB runtime
  6.6 MB net/http
  3.1 MB net
  3.0 MB crypto/tls
  2.0 MB reflect
  1.4 MB math/big
  1.3 MB crypto/x509
  935 kB os
  ...

With this change:

  7.9 MB runtime
  3.1 MB net
  2.0 MB reflect
  935 kB os
  ...

Slight modifications to avoid importing net/http reduce the binary size significantly.

Signed-off-by: Christian Stewart <christian@aperture.us>
@mafredri
Copy link
Member

Hey @paralin. Thanks for this proposal, the weight reduction is impressive but I believe this is an uphill battle. For instance, in #373 there's a request to introduce *http.Client as well as make the DialOptions struct the same for both wasm and non-wasm.

We also want to avoid breaking the API while we're v1, targeting API breakages for a potential v2 in the future is of course an option, but will not be done lightly.

With these constraints in mind, I don't see how we could support this. The only option I can think of is to let users opt-in via a build tag, say GOOS=js GOARCH=wasm go build -tags slim. As a result, we could radically change the wasm API and simplify the return values.

But before we consider adding more complexity to the code-base, I'd like to hear about use-cases where this weight reduction is necessary.

@paralin
Copy link
Contributor Author

paralin commented Aug 21, 2024

In a web browser app using wasm with Go, any weight reduction is significant.

While going through the list of packages taking up space in the binary, net/http was at the top of the list.

6.6 MB net/http

As of January 2024, the average download speed globally for mobile internet was 50 Mbps

That's around 1 second of time spent downloading net/http, and even a second delay loading a website feels like a lot.

Of course gzip / brotli compression reduces this significantly.

I understand the desire to keep API compatibility. I also use fetch on wasm to reduce binary size: https://github.com/aperturerobotics/util/blob/master/js/fetch/fetch.go

@mafredri
Copy link
Member

@paralin what you say makes sense but what I'm looking for is concrete use-cases. Code where you're using websocket and building for wasm, without importing anything from net/http.

If I'd write a program only using websocket.Dial, the difference wouldn't be all that big:

❯ ls -lha
total 7.1M
drwxr-xr-x 2 coder coder 4.0K Aug 21 12:24 ./
drwxr-xr-x 5 coder coder 4.0K Aug 21 12:24 ../
-rwxr-xr-x 1 coder coder 3.1M Aug 21 12:24 dial-master*
-rwxr-xr-x 1 coder coder 897K Aug 21 12:24 dial-master.gz*
-rwxr-xr-x 1 coder coder 2.4M Aug 21 12:24 dial-pr*
-rwxr-xr-x 1 coder coder 704K Aug 21 12:24 dial-pr.gz*
-rw-r--r-- 1 coder coder  271 Aug 21 12:23 main.go

Sure, 0.7M is still significant, as is 193K gzipped. The thing we need to evaluate though is if the cost of supporting this use-case is worth the gain. How big will the gain be? How easy is it to write and maintain a program with no imports from http, etc? And that's where real-world use-cases come in and would help a lot.

@paralin
Copy link
Contributor Author

paralin commented Aug 21, 2024

The concrete use case is compiling a wasm binary and running it in a webpage, then using websocket.Dial. It's quite easy to make a program that dials a websocket and exchanges messages with a server in wasm that doesn't otherwise use net/http.

That said if it requires a separate go tag I probably wouldn't use the feature and would just keep using my forked version. No worries if you don't feel that the 0.7MB difference is worth it. It definitely is in wasm apps though, and as wasm and go become more developed and widely used, this kind of size savings will become more valuable. At the moment it probably doesn't matter to anyone that wouldn't just fork the package for their use case anyway.

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