Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

fix: avoid importing net/http on wasm#449

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
paralin wants to merge2 commits intocoder:master
base:master
Choose a base branch
Loading
fromparalin:remove-net-http-wasm

Conversation

@paralin
Copy link
Contributor

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.

@nhooyrnhooyr changed the base branch frommaster todevMay 5, 2024 01:35
Copy link
Contributor

@nhooyrnhooyr 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
ContributorAuthor

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

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.

@mafredrimafredri deleted the branchcoder:masterAugust 15, 2024 20:35
@mafredrimafredri reopened thisAug 16, 2024
@mafredrimafredri changed the base branch fromdev tomasterAugust 16, 2024 14:41
Using goweight:https://github.com/paralin/goweightGOOS=js GOARCH=wasm goweight | lessWithout 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 theDialOptions 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, sayGOOS=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
ContributorAuthor

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 usingwebsocket and building for wasm, without importing anything fromnet/http.

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

❯ ls -lhatotal 7.1Mdrwxr-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 is193K 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 fromhttp, etc? And that's where real-world use-cases come in and would help a lot.

@paralin
Copy link
ContributorAuthor

paralin commentedAug 21, 2024
edited
Loading

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 freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

1 more reviewer

@nhooyrnhooyrnhooyr left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@paralin@nhooyr@mafredri

[8]ページ先頭

©2009-2025 Movatter.jp