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

cmd/compile: ssa.a contains the string SetsockoptIPv6Mreq #20084

Closed
josharian opened this issue Apr 23, 2017 · 7 comments

Comments

Projects
None yet
5 participants
@josharian
Copy link
Contributor

commented Apr 23, 2017

$ grep -c "SetsockoptIPv6Mreq" $GOROOT/pkg/darwin_amd64/cmd/compile/internal/ssa.a
1

This is a bit surprising. How did it get there?

There are a few places in the ssa package where we have types that refer to *os.File, like map[string]*os.File. When exporting, we re-export *os.File, which is struct { *os.file }.

os.file is (on my machine) struct { os.pfd poll.FD; os.name string; os.dirinfo *os.dirInfo; os.nonblock bool }.

poll.FD is struct { poll.fdmu poll.fdMutex; Sysfd int; poll.pd poll.pollDesc; poll.iovecs *[]syscall.Iovec; IsStream bool; ZeroReadIsEOF bool }.

And so on all the way down to SetsockoptIPv6Mreq, leaving a trail of types in our wake.

I've been trying to think of how the compiler might possibly need these types in a package that imports only cmd/compile/internal/ssa. It doesn't need them directly for typechecking, because os.file is not exported, and thus unreachable via our code. The struct size/shape/alignment doesn't matter, because it is only reference is via a pointer (*os.file); same reasoning for GC maps.

I'd love enlightenment.

If they're not needed, we should figure out how to get rid of them. Maybe part of the problem in #20070 is that our object files have lots of stuff in them that we don't even need in the first place?

@griesemer @mdempsky

@josharian josharian added the ToolSpeed label Apr 23, 2017

@josharian josharian added this to the Go1.9 milestone Apr 23, 2017

@josharian josharian changed the title cmd/compile: ssa object file contains the string SetsockoptIPv6Mreq cmd/compile: ssa.a contains the string SetsockoptIPv6Mreq Apr 23, 2017

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2017

To help quantify, CL 41493 contains a quick hack to remove explicit references to *os.File from package SSA. It results in a 10% reduction in export data size for the package.

josharian added a commit to josharian/go that referenced this issue Apr 23, 2017

cmd/compile: remove references to *os.File from ssa package
DO NOT SUBMIT

This is a demo CL for golang#20084.
It currently reduces the size of the ssa export data
by 10%, from 76154 to 67886.

Change-Id: I49e8951c5bfce63ad2b7f4fc3bfa0868c53114f9
@griesemer

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2017

This seems like another good example of a situation where using an interface to abstract enormous amounts of detail (which in turn turns out as type information) would help. Having an abstract File notion (which has come up many times before) would probably avoid pulling in these details.

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2017

@griesemer agreed, although it'd also be better for the compiler to handle this more gracefully as well.

I spent some time on this today but didn't make much progress. There are at least a few things this apparently inaccessible data is being used for: (1) method sets, (2) hash/eq routines, (3) sanity checks across imported packages, (4) re-typechecking of inlined method bodies. (1) and (2) could be represented more parsimoniously. (3) is nice but substantively dropping the cost of compilation is probably nicer. (4) could possibly be worked around.

I wonder whether the sanity checks could be protected behind a flag, in favor of not constructing the type the second time around, and if so, how much that would save in practice.

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2017

I just updated CL 41493 with some numbers: This improved compilation time for a downstream package (cmd/compile/internal/amd64) by > 3%. Over a large compilation graph, like benchjuju has, this could be significant.

Moving to 1.10, though, because I don't see this happening in the next week.

@josharian josharian modified the milestones: Go1.10, Go1.9 Apr 24, 2017

gopherbot pushed a commit that referenced this issue Apr 24, 2017

cmd/compile: remove references to *os.File from ssa package
This reduces the size of the ssa export data
by 10%, from 76154 to 67886.

It doesn't appear that #20084, which would do this automatically,
is going to be fixed soon. Do it manually for now.

This speeds up compiling cmd/compile/internal/amd64
and presumably its comrades as well:

name          old time/op       new time/op       delta
CompileAMD64       89.6ms ± 6%       86.7ms ± 5%  -3.29%  (p=0.000 n=49+47)

name          old user-time/op  new user-time/op  delta
CompileAMD64        116ms ± 5%        112ms ± 5%  -3.51%  (p=0.000 n=45+42)

name          old alloc/op      new alloc/op      delta
CompileAMD64       26.7MB ± 0%       25.8MB ± 0%  -3.26%  (p=0.008 n=5+5)

name          old allocs/op     new allocs/op     delta
CompileAMD64         223k ± 0%         213k ± 0%  -4.46%  (p=0.008 n=5+5)

Updates #20084

Change-Id: I49e8951c5bfce63ad2b7f4fc3bfa0868c53114f9
Reviewed-on: https://go-review.googlesource.com/41493
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Apr 25, 2017

CL https://golang.org/cl/41493 mentions this issue.

josharian added a commit to josharian/go that referenced this issue Apr 26, 2017

cmd/compile: remove references to *os.File from ssa package
This reduces the size of the ssa export data
by 10%, from 76154 to 67886.

It doesn't appear that golang#20084, which would do this automatically,
is going to be fixed soon. Do it manually for now.

This speeds up compiling cmd/compile/internal/amd64
and presumably its comrades as well:

name          old time/op       new time/op       delta
CompileAMD64       89.6ms ± 6%       86.7ms ± 5%  -3.29%  (p=0.000 n=49+47)

name          old user-time/op  new user-time/op  delta
CompileAMD64        116ms ± 5%        112ms ± 5%  -3.51%  (p=0.000 n=45+42)

name          old alloc/op      new alloc/op      delta
CompileAMD64       26.7MB ± 0%       25.8MB ± 0%  -3.26%  (p=0.008 n=5+5)

name          old allocs/op     new allocs/op     delta
CompileAMD64         223k ± 0%         213k ± 0%  -4.46%  (p=0.008 n=5+5)

Updates golang#20084

Change-Id: I49e8951c5bfce63ad2b7f4fc3bfa0868c53114f9
@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2017

The string "SetsockoptIPv6Mreq" is not there anymore with tip (2f588ff).

$ ../bin/go version
go version devel +2f588ff08f Wed Nov 29 20:40:41 2017 +0000 darwin/amd64
$ grep -c "SetsockoptIPv6Mreq" ../pkg/darwin_amd64/cmd/compile/internal/ssa.a
0

Is there anything else expected to be done in this issue? @josharian

@mdempsky

This comment has been minimized.

Copy link
Member

commented Nov 30, 2017

This was "fixed" by 4ee934a. The more general issue will be fixed by #20070.

@mdempsky mdempsky closed this Nov 30, 2017

@golang golang locked and limited conversation to collaborators Nov 30, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.