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

net/http/httptrace: uses reflect.Value.Call, forces linker's deadcode to be conservative #38505

Closed
bradfitz opened this issue Apr 17, 2020 · 3 comments
Milestone

Comments

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 17, 2020

net/http/httptrace uses reflect.Value.Call:

https://github.com/golang/go/blob/release-branch.go1.14/src/net/http/httptrace/trace.go#L201

Its use anywhere in a program forces the linker's deadcode pass to go into its conservative mode when deciding whether to preserve otherwise-unused exported methods.

Because net/http depends on httptrace, any net/http program is potentially larger than it needs to be, even if the caller program doesn't use httptrace. That then makes binaries bigger (138 KiB larger in one program here)

(People who like this bug might also like #38450)

We should probably rewrite (or auto-generate?) httptrace to not use reflect.Value.Call.

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Apr 18, 2020

I think this was fixed by https://go-review.googlesource.com/c/go/+/228792

But that might get reverted due to #38515, depending on how it gets fixed.

https://go-review.googlesource.com/c/go/+/228878 adds tests that reflect.Value.Call stops making the linker do this, though, so this bug can likely be closed once CL 228878 is in and #38515 is fixed.

@bradfitz bradfitz added this to the Go1.15 milestone Apr 18, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 18, 2020

Change https://golang.org/cl/228878 mentions this issue: cmd/link: add a test that reflect.Value.Call does not bring methods live

gopherbot pushed a commit that referenced this issue Apr 18, 2020
reflect.Value.Call, if reachable, used to bring all exported
methods live. CL 228792 fixes this, removing the check of
reflect.Value.Call. This CL adds a test.

Updates #38505.

Change-Id: Ib4cab3c3c86c9c9702d041266e59b159d0ff0a97
Reviewed-on: https://go-review.googlesource.com/c/go/+/228878
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Apr 19, 2020

Okay, we can close this now.

It's also not as bad as I originally thought: a program that imported net/http but didn't use httptrace.WithClientTrace didn't end up going into conservative reflectSeen mode.

But with the latest toolchain changes (notably https://go-review.googlesource.com/c/go/+/228792), even using WithClientTrace shouldn't trigger the linker's reflectSeen, as it doesn't do any method lookups.

@bradfitz bradfitz closed this Apr 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.