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.

Sign up for GitHub

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

Jump to bottom

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
Closed

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

bradfitz opened this issue Apr 17, 2020 · 3 comments
Labels
FrozenDueToAge
Milestone
Go1.15

Comments

@bradfitz
Copy link
Contributor

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

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

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
@cherrymui
cmd/link: add a test that reflect.Value.Call does not bring methods live
de2318e 
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

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.

@golang golang locked and limited conversation to collaborators Apr 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.15
Development

No branches or pull requests
2 participants
@bradfitz @gopherbot