proposal: cmd/compile: compiler-emitted itab symbols #14874
Comments
|
CL https://golang.org/cl/20902 mentions this issue. |
|
CL https://golang.org/cl/20888 mentions this issue. |
|
CL https://golang.org/cl/20901 mentions this issue. |
|
CL https://golang.org/cl/20889 mentions this issue. |
|
CL https://golang.org/cl/20900 mentions this issue. |
walken-google
changed the title
ianlancetaylor
changed the title
|
CC @randall77 |
|
This is written as a proposal, but if you are seeing better code and a size reduction, it seems uncontroversial. This should go in. |
|
I want to check that I understand how this works in the presence of shared libraries. Suppose libraries libT.so, libI.so, libX.so, and libY.so, where the only interdependencies between them are that libX.so and libY.so both depend on both of libT.so and libI.so. Suppose libT.so declares type t.T, libI.so declares interface i.I (that t.T happens to implement), and libX.so and libY.so both include conversions from t.T to i.I. Since libX.so and libY.so don't know about each other, they both need to provide definitions for go.itab.t.T,i.I. How is this duplicity handled? Will one definition take priority and always be canonically used, or do we need to ensure the runtime / compiler-emitted code are safe in the presence of redundant itabs? I notice, for example, that ifaceeq currently requires identical itab pointer values. So if we need to worry about redundant itabs, we need to at least relax that function to check for itab._type equality instead (maybe only in the shared library case). |
|
I'm not entirely sure about shared libraries. In the single-executable case, what happens is that packages X and Y will both emit a go.itab.t.T,i.I symbol which is marked as dupok. The linker then notices the two same-named symbols, throws one, keeps the other, and we end up with a unique itab in the final binary (I actually tested this). In the case of shared libraries, I'm not sure how dupok symbols are handled, but I would assume they have to be handled already anyway - before my changes, libX.so and libY.so already both define go.type.t.T and they already need to end up with a single symbol being used. I don't quite remember ELF semantics but I think what happens is that the linker loads shared libraries in a certain order and if one redefines a previously linked global symbol, the new definition is thrown out and the symbol is made to point to the previous definition instead (essentially giving dupok semantics to all ELF global symbols). |
|
Yeah, the static executable case is easy and seems safe to me. I think it should be okay with at most simple fixes for the shared library case too, but I'd just like to confirm that first. (cc @mwhudson) |
|
Yes, I think it should be OK, as @walken-google says, the first symbol of a given names "wins" (at least for global symbols with default visibility), so that should be OK. It might be another thing for whichever brave soul implements plugins to think about, but I think shared libraries should be fine. |
|
keith randall suggested that since convT2I now has an itab argument, we don't need the src type argument anymore (that's already in the itab). Removing that argument saves another 0.1% space in the binaries I tried - the savings are now ~1.3% for bin/{gen,go,gofmt} binaries and ~1.1% for the pkg/tool/linux_amd64/* binaries. |
|
FWIW, we know at compile time whether the final argument to convT2I will be nil, so we could in theory have two convT2I flavors, the most commonly used of which has no final arg. |
See #14874 This change makes the runtime register all compiler generated itabs (as obtained from the moduledata) during init. Change-Id: I9969a0985b99b8bda820a631f7fe4c78f1174cdf Reviewed-on: https://go-review.googlesource.com/20900 Reviewed-by: Keith Randall <khr@golang.org> Run-TryBot: Michel Lespinasse <walken@google.com>
See #14874 This change tells the compiler to emit itab and itablink symbols in situations where they could be useful; however the compiled code does not actually make use of the new symbols yet. Change-Id: I0db3e6ec0cb1f3b7cebd4c60229e4a48372fe586 Reviewed-on: https://go-review.googlesource.com/20888 Reviewed-by: David Crawshaw <crawshaw@golang.org> Run-TryBot: Michel Lespinasse <walken@google.com>
See #14874 This change tells the linker to collect all the itablink symbols and collect them so that moduledata can have a slice of all compiler generated itabs. The logic is shamelessly adapted from what is done with typelink symbols. Change-Id: Ie93b59acf0fcba908a876d506afbf796f222dbac Reviewed-on: https://go-review.googlesource.com/20889 Reviewed-by: Keith Randall <khr@golang.org>
…aped See #14874 This change adds a compiler optimization for pointer shaped convT2I. Since itab symbols are now emitted by the compiler, the itab address can be directly moved into the iface structure. Change-Id: I311483af544519ca682c5f872960717ead772f26 Reviewed-on: https://go-review.googlesource.com/20901 Reviewed-by: Keith Randall <khr@golang.org> Reviewed-by: David Crawshaw <crawshaw@golang.org>
See #14874 Updates #6853 This change adds a compiler optimization for non pointer shaped convT2I. Since itab symbols are now emitted by the compiler, the itab address can be passed directly to convT2I instead of passing the iface type and a cache pointer argument. Compilebench results for the 5-commits series ending here: name old time/op new time/op delta Template 336ms ± 4% 344ms ± 4% +2.61% (p=0.027 n=9+8) Unicode 165ms ± 6% 173ms ± 7% +5.11% (p=0.014 n=9+9) GoTypes 1.09s ± 1% 1.06s ± 2% -3.29% (p=0.000 n=9+9) Compiler 5.09s ±10% 4.75s ±10% -6.64% (p=0.011 n=10+10) MakeBash 31.1s ± 5% 30.3s ± 3% ~ (p=0.089 n=10+10) name old text-bytes new text-bytes delta HelloSize 558k ± 0% 558k ± 0% +0.02% (p=0.000 n=10+10) CmdGoSize 6.24M ± 0% 6.11M ± 0% -2.11% (p=0.000 n=10+10) name old data-bytes new data-bytes delta HelloSize 3.66k ± 0% 3.74k ± 0% +2.41% (p=0.000 n=10+10) CmdGoSize 134k ± 0% 162k ± 0% +20.76% (p=0.000 n=10+10) name old bss-bytes new bss-bytes delta HelloSize 126k ± 0% 126k ± 0% ~ (all samples are equal) CmdGoSize 149k ± 0% 146k ± 0% -2.17% (p=0.000 n=10+10) name old exe-bytes new exe-bytes delta HelloSize 924k ± 0% 924k ± 0% +0.05% (p=0.000 n=10+10) CmdGoSize 9.77M ± 0% 9.62M ± 0% -1.47% (p=0.000 n=10+10) Change-Id: Ib230ddc04988824035c32287ae544a965fedd344 Reviewed-on: https://go-review.googlesource.com/20902 Reviewed-by: Keith Randall <khr@golang.org> Reviewed-by: David Crawshaw <crawshaw@golang.org> Run-TryBot: Michel Lespinasse <walken@google.com>
I am writing this after the fact, as I already have a proposed patch set out for review. Sorry about that.
itab structures are currently always generated by the runtime. This leads to inefficiencies when converting between a known type and a known interface - the generated code could be more efficient if it could know the itab address, but it can't if the itab is generated by the runtime, so we end up generating more complicated code that calls typ2Itab and caches the result.
I propose having the compiler generate itab structures when the go code converts between a known type and a known interface, so that the generated itabs can be taken as constant addresses in the generated code. The runtime will register these structures into its itab hash tables at init time, and is still capable of creating more on the fly if required during conversions from one interface type to another.
sample code:
compiled with master at fc6bcde:
Compiled with above plus the proposed changes:
The generated code for T2I is now slightly simpler because it has one less constant argument to convT2I to push on the stack - this saves 12 bytes of code for each place that converts T to I, at the cost of having the compiler generate an itab (once) which takes 32 bytes + 8 per interface method.
The generated code for TP2I is now much simpler, just moving the (known) itab pointer and the T pointer into the returned iface. In this simple case, this saves so much code that it pays for the itab size on the first use. In a more complex function, I suspect the benefits will be less but there should be an overall size reduction either way.
I am also seeing a ~1.2% size reduction for the bin/{gen,go,gofmt} binaries and a ~1.0% size reduction for the pkg/tool/linux_amd64/* binaries.
I do not have meaningful benchmarking numbers to report at this point (my results are way too noisy for such a small expected change).
The text was updated successfully, but these errors were encountered: