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/link: include per-package aggregate static temp symbols? #39053

Open
bradfitz opened this issue May 13, 2020 · 9 comments
Open

cmd/link: include per-package aggregate static temp symbols? #39053

bradfitz opened this issue May 13, 2020 · 9 comments
Milestone

Comments

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented May 13, 2020

I've been working on a Go binary size analysis tool that aims to attribute ~each byte of the binary back to a function or package.

One thing it's not great at doing at the moment in the general case is summing the size of static temp (..stmp_NNN) because those symbols are removed by default (except in external linking mode).

Worse, with Macho-O binaries not having sizes on symbols, I can't even accurately count the sizes of symbols that do exist because the stmp values are in the DATA, but lacking symbols, so I end up calculating the wrong size of existing symbols:

e.g. this float64info should actually be the same size as the float32info, but a bunch of stmp_NNN are omitted between float64info and hash..inittask:

   sym "debug/dwarf._Class_index" (at 19138544), size=464
   sym "strconv.float32info" (at 19139008), size=32
   sym "strconv.float64info" (at 19139040), size=1664
   sym "hash..inittask" (at 19140704), size=32
   sym "internal/bytealg..inittask" (at 19140736), size=32
   sym "internal/reflectlite..inittask" (at 19140768), size=32
   sym "internal/singleflight..inittask" (at 19140800), size=32

Looking at a binary with the stmp symbols, I get the accurate size for float64info:

   sym "unicode..stmp_539" (at 1005d4260), size=32
   sym "unicode..stmp_553" (at 1005d4280), size=32
   sym "unicode..stmp_558" (at 1005d42a0), size=32
   sym "crypto..stmp_2" (at 1005d42c0), size=32
   sym "crypto/tls..stmp_205" (at 1005d42e0), size=32
   sym "strconv.float32info" (at 1005d4300), size=32
   sym "strconv.float64info" (at 1005d4320), size=32
   sym "unicode..stmp_11" (at 1005d4340), size=32
   sym "unicode..stmp_113" (at 1005d4360), size=32
   sym "unicode..stmp_121" (at 1005d4380), size=32
   sym "unicode..stmp_147" (at 1005d43a0), size=32
   sym "unicode..stmp_150" (at 1005d43c0), size=32

So, my request: can we aggregate all the stmp_NNN symbols together per-package and emit one symbol per section per Go package, like:

        unicode..stmp_pkg

Then I can both calculate the sum stmp sizes per package (e.g. unicode is 68KB, crypto/tls is 12KB), and I can also accurately calculate the size of other symbols (Mach-o symbols without a size)

This would make binaries a tiny bit bigger (but bounded by number of packages at least) but would permit more analysis at making them much smaller, IMO. (Or output it to a separate file.)

It would also require sorting the stmp values all together in the binary. They're currently scattered around:

   sym "vendor/golang.org/x/net/route.rtmVersion" (at 100615964), size=2
   sym "runtime..stmp_40" (at 100615966), size=2
   sym "context.goroutines" (at 100615968), size=4
   sym "runtime..stmp_41" (at 10061596c), size=4
   sym "runtime.argc" (at 100615970), size=4
   sym "runtime.crashing" (at 100615974), size=4

(Note a context symbol between runtime.stmp_40 and stmp_41 )

What I'd like to see is something like:

   sym "vendor/golang.org/x/net/route.rtmVersion" (at 100615964), size=2
   sym "context.goroutines" (at 100615966), size=4
   sym "runtime..stmp_pkg" (at 100615970), size=6
   sym "runtime.argc" (at 100615970), size=4
   sym "runtime.crashing" (at 100615974), size=4

Thoughts? Alternatives?

/cc @ianlancetaylor (who schooled me on some of this), @cherrymui, @thanm, @randall77, @josharian, @jeremyfaller

@thanm
Copy link
Member

@thanm thanm commented May 13, 2020

Aggregating stmp's seems as though it would potentially interfere with linker dead code elimination, since if any stmp in the aggregated glob is made live (by an access from a function presumably) then the entire blob is live.

Seems as though you would get better data if you wrote some throwaway code in the linker, as opposed to relying on the symbol table. There are lots of interesting syms these days that don't go into the symbol table. During 1.15 in the new linker we converted quite a lot of the function-specific DWARF symbols into anonymous syms (which makes them invisible to any tool that keys off the symtab).

@thanm
Copy link
Member

@thanm thanm commented May 13, 2020

NB: in the linker today when field tracking is enabled, the linker's dead code elimination pass builds a "Reachparent" graph where each edge (X -> Y) indicates that symbol X was marked live because symbol Y was live. You could hijack this same mechanism for your purposes, with some additional hacking I think (for "new" syms that the linker invents from nothing). I can send you pointers/examples if you want to try prototyping this.

@josharian
Copy link
Contributor

@josharian josharian commented May 13, 2020

This wouldn’t play well with making static temp symbols content-addressable, which I have long wanted to do. (Can’t find the issue now.)

We don’t make nearly enough things content-addressable, in general. (Stkobj, short strings, type algs, more.)

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented May 13, 2020

We could do the grouping at link time, instead of compile time, after dead code elimination.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented May 13, 2020

@josharian Certainly I'd also like making more things content-addressable. I think this is one active area that we'll keep work on for the "new linker" work.

Are static temps always read-only? I think there are mutable static temps? (I may be wrong.)

@thanm
Copy link
Member

@thanm thanm commented May 13, 2020

@cherrymui good point.

Clumping together stmp by package could also introduce some padding between clumps, but that doesn't seem too scary.

@josharian
Copy link
Contributor

@josharian josharian commented May 13, 2020

@cherrymui I think it’d be better to set up content addressability in the compiler, but having the linker do it might be a good backstop and/or source of cheap wins. I think all statictmps are readonly, but we could be extra safe and only do symbols explicitly marked as readonly (and hidden).

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented May 13, 2020

Yeah, it should definitely be done in the compiler. I think there will also need linker changes to make it work. (To be clear the "new linker" work is not limited to changes in cmd/link.)

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented May 14, 2020

@josharian I don't think all static temps are read-only, for example, https://play.golang.org/p/KJ62e1qkOHN

Here, x points to a static temp, which is mutable.

"".x SDATA size=8
	0x0000 00 00 00 00 00 00 00 00                          ........
	rel 0+8 t=1 ""..stmp_0+0
""..stmp_0 SNOPTRDATA size=80
	0x0000 01 00 00 00 00 00 00 00 02 00 00 00 00 00 00 00  ................
	0x0010 03 00 00 00 00 00 00 00                          ........
@cagedmantis cagedmantis added this to the Backlog milestone May 18, 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
5 participants
You can’t perform that action at this time.