code review 6856126: cmd/gc: do not export useless private symbols.

cmd/gc: do not export useless private symbols.

Fixes issue 4252.

[remyoudompheng, 2012-12-01 16:55:24]

Hello rsc@golang.org, golang-dev@googlegroups.com (cc:
golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[remyoudompheng, 2012-12-01 17:04:08]

The CL should reduce the include craziness in Go builds. 

It is difficult to measure the resulting performance variation but on x86-64

Before:
perf stat bin/go install -a std -> 54.2G instructions
time bin/go install -a std -> 28.97s user, 8.17s system

After:
perf stat bin/go install -a std -> 48.3G instructions
time bin/go install -a std -> 25.93s user, 7.81s system

Lines of export data (ar pf xxx.a __.PKGDEF | wc -l) on some packages:

             Before  After
encoding/asn1  418     35
fmt            358     44
net            758    360 (linux/amd64)
net/http/pprof 606    361
unicode        877    241

ar pf fmt.a | wc -l: 358
ar pf

[aam, 2012-12-01 17:29:57]

just a style thing, no other comments from me.

https://codereview.appspot.com/6856126/diff/5001/src/cmd/gc/export.c
File src/cmd/gc/export.c (right):

https://codereview.appspot.com/6856126/diff/5001/src/cmd/gc/export.c#newcode161
src/cmd/gc/export.c:161: if (t && t->sym && t->sym->def && !exportedsym(t->sym))
{
if( like you did above.

[DMorsing, 2012-12-01 18:07:00]

https://codereview.appspot.com/6856126/diff/5001/src/cmd/gc/lex.c
File src/cmd/gc/lex.c (right):

https://codereview.appspot.com/6856126/diff/5001/src/cmd/gc/lex.c#newcode2040
src/cmd/gc/lex.c:2040: s->origpkg = builtinpkg;
What about the table driven symbols and the symbols declared in 5g, 6g, 8g?

[dfc, 2012-12-02 08:41:29]

On 2012/12/01 18:07:00, DMorsing wrote:
> https://codereview.appspot.com/6856126/diff/5001/src/cmd/gc/lex.c
> File src/cmd/gc/lex.c (right):
> 
> https://codereview.appspot.com/6856126/diff/5001/src/cmd/gc/lex.c#newcode2040
> src/cmd/gc/lex.c:2040: s->origpkg = builtinpkg;
> What about the table driven symbols and the symbols declared in 5g, 6g, 8g?

Here is a datapoint from linux/arm. This was the 5g invocation compiling pkg/net

before:

Command being timed: "/home/dfc/go/pkg/tool/linux_arm/5g -o
/tmp/go-build199703639/net/_obj/_go_.5 -p net -D _/home/dfc/go/src/pkg/net -I
/tmp/go-build199703639 ./dial.go ./dnsclient.go ./dnsclient_unix.go
./dnsconfig_unix.go ./dnsmsg.go ./fd_linux.go ./fd_unix.go ./file_unix.go
./hosts.go ./interface.go ./interface_linux.go ./ip.go ./iprawsock.go
./iprawsock_posix.go ./ipsock.go ./ipsock_posix.go ./lookup.go ./lookup_unix.go
./mac.go ./net.go ./newpollserver_unix.go ./parse.go ./pipe.go ./port.go
./port_unix.go ./sendfile_linux.go ./sock_linux.go ./sock_posix.go
./sockopt_linux.go ./sockopt_posix.go ./sockoptip_linux.go ./sockoptip_posix.go
./tcpsock.go ./tcpsock_posix.go ./udpsock.go ./udpsock_posix.go ./unixsock.go
./unixsock_posix.go /tmp/go-build199703639/net/_obj/_cgo_gotypes.go
/tmp/go-build199703639/net/_obj/cgo_linux.cgo1.go
/tmp/go-build199703639/net/_obj/cgo_unix.cgo1.go"
User time (seconds): 3.85
System time (seconds): 0.69
Percent of CPU this job got: 99%
Elapsed (wall clock) time (h:mm:ss or m:ss): 0:04.57
Average shared text size (kbytes): 0
Average unshared data size (kbytes): 0
Average stack size (kbytes): 0
Average total size (kbytes): 0
Maximum resident set size (kbytes): 435184
Average resident set size (kbytes): 0
Major (requiring I/O) page faults: 0
Minor (reclaiming a frame) page faults: 27255
Voluntary context switches: 0
Involuntary context switches: 513
Swaps: 0
File system inputs: 0
File system outputs: 0
Socket messages sent: 0
Socket messages received: 0
Signals delivered: 0
Page size (bytes): 4096
Exit status: 0

after:

Command being timed: "/home/dfc/go/pkg/tool/linux_arm/5g -o
/tmp/go-build199703639/net/_obj/_go_.5 -p net -D _/home/dfc/go/src/pkg/net -I
/tmp/go-build199703639 ./dial.go ./dnsclient.go ./dnsclient_unix.go
./dnsconfig_unix.go ./dnsmsg.go ./fd_linux.go ./fd_unix.go ./file_unix.go
./hosts.go ./interface.go ./interface_linux.go ./ip.go ./iprawsock.go
./iprawsock_posix.go ./ipsock.go ./ipsock_posix.go ./lookup.go ./lookup_unix.go
./mac.go ./net.go ./newpollserver_unix.go ./parse.go ./pipe.go ./port.go
./port_unix.go ./sendfile_linux.go ./sock_linux.go ./sock_posix.go
./sockopt_linux.go ./sockopt_posix.go ./sockoptip_linux.go ./sockoptip_posix.go
./tcpsock.go ./tcpsock_posix.go ./udpsock.go ./udpsock_posix.go ./unixsock.go
./unixsock_posix.go /tmp/go-build199703639/net/_obj/_cgo_gotypes.go
/tmp/go-build199703639/net/_obj/cgo_linux.cgo1.go
/tmp/go-build199703639/net/_obj/cgo_unix.cgo1.go"
User time (seconds): 3.46
System time (seconds): 0.63
Percent of CPU this job got: 99%
Elapsed (wall clock) time (h:mm:ss or m:ss): 0:04.10
Average shared text size (kbytes): 0
Average unshared data size (kbytes): 0
Average stack size (kbytes): 0
Average total size (kbytes): 0
Maximum resident set size (kbytes): 395920
Average resident set size (kbytes): 0
Major (requiring I/O) page faults: 0
Minor (reclaiming a frame) page faults: 24801
Voluntary context switches: 0
Involuntary context switches: 75
Swaps: 0
File system inputs: 0
File system outputs: 0
Socket messages sent: 0
Socket messages received: 0
Signals delivered: 0
Page size (bytes): 4096
Exit status: 0

[lvd2, 2012-12-03 22:40:29]

https://codereview.appspot.com/6856126/diff/5001/test/declbuiltin.dir/a.go
File test/declbuiltin.dir/a.go (right):

https://codereview.appspot.com/6856126/diff/5001/test/declbuiltin.dir/a.go#ne...
test/declbuiltin.dir/a.go:14: func Test() {
shouldnt main.go run this one?

[rsc, 2012-12-06 05:47:53]

LGTM

Code looks good as long as it works. This is all very subtle.
Can you move the declbuiltin.dir to fixedbugs/bugNNN.dir?
I'd like to avoid new subdirectories of test/.
Please wait until the pending comments by others are addressed.
Thanks.

[remyoudompheng, 2012-12-06 07:22:51]

Hello rsc@golang.org, golang-dev@googlegroups.com, mirtchovski@gmail.com,
daniel.morsing@gmail.com, dave@cheney.net, lvd@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[remyoudompheng, 2012-12-06 07:23:55]

Still have to take a look at DMorsing's comment.

http://codereview.appspot.com/6856126/diff/5001/src/cmd/gc/export.c
File src/cmd/gc/export.c (right):

http://codereview.appspot.com/6856126/diff/5001/src/cmd/gc/export.c#newcode161
src/cmd/gc/export.c:161: if (t && t->sym && t->sym->def && !exportedsym(t->sym))
{
On 2012/12/01 17:29:57, aam wrote:
> if( like you did above.

Done.

http://codereview.appspot.com/6856126/diff/5001/test/declbuiltin.dir/a.go
File test/declbuiltin.dir/a.go (right):

http://codereview.appspot.com/6856126/diff/5001/test/declbuiltin.dir/a.go#new...
test/declbuiltin.dir/a.go:14: func Test() {
On 2012/12/03 22:40:30, lvd2 wrote:
> shouldnt main.go run this one?

Done.

[dfc, 2012-12-07 00:37:38]

On an old arm5 host this change shaved 300ms and 10mb off the build time for
pkg/net

dfc@qnap:~/go/src/pkg/net$ /usr/bin/time -v go build
        Command being timed: "go build"
        User time (seconds): 4.37
        System time (seconds): 0.48
        Percent of CPU this job got: 96%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:05.01
        Average shared text size (kbytes): 0
        Average unshared data size (kbytes): 0
        Average stack size (kbytes): 0
        Average total size (kbytes): 0
        Maximum resident set size (kbytes): 108208
        Average resident set size (kbytes): 0
        Major (requiring I/O) page faults: 0
        Minor (reclaiming a frame) page faults: 60003
        Voluntary context switches: 320
        Involuntary context switches: 219
        Swaps: 0
        File system inputs: 0
        File system outputs: 8088
        Socket messages sent: 0
        Socket messages received: 0
        Signals delivered: 0
        Page size (bytes): 4096
        Exit status: 0

after:

dfc@qnap:~/go/src/pkg/net$ /usr/bin/time -v go build
        Command being timed: "go build"
        User time (seconds): 4.09
        System time (seconds): 0.47
        Percent of CPU this job got: 95%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:04.75
        Average shared text size (kbytes): 0
        Average unshared data size (kbytes): 0
        Average stack size (kbytes): 0
        Average total size (kbytes): 0
        Maximum resident set size (kbytes): 98436
        Average resident set size (kbytes): 0
        Major (requiring I/O) page faults: 1
        Minor (reclaiming a frame) page faults: 57511
        Voluntary context switches: 322
        Involuntary context switches: 216
        Swaps: 0
        File system inputs: 208
        File system outputs: 7896
        Socket messages sent: 0
        Socket messages received: 0
        Signals delivered: 0
        Page size (bytes): 4096
        Exit status: 0

The results for the whole stdlibrary are even more impressive

before:

dfc@qnap:~/go/src$ /usr/bin/time -v go build -a std
        Command being timed: "go build -a std"
        User time (seconds): 91.40
        System time (seconds): 10.33
        Percent of CPU this job got: 94%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 1:47.15
        Average shared text size (kbytes): 0
        Average unshared data size (kbytes): 0
        Average stack size (kbytes): 0
        Average total size (kbytes): 0
        Maximum resident set size (kbytes): 195004
        Average resident set size (kbytes): 0
        Major (requiring I/O) page faults: 2
        Minor (reclaiming a frame) page faults: 1036733
        Voluntary context switches: 2608
        Involuntary context switches: 1930
        Swaps: 0
        File system inputs: 848
        File system outputs: 295064
        Socket messages sent: 0
        Socket messages received: 0
        Signals delivered: 0
        Page size (bytes): 4096
        Exit status: 0

after:

		dfc@qnap:~/go/src$ /usr/bin/time -v go build -a std
		        Command being timed: "go build -a std"
		        User time (seconds): 81.01
		        System time (seconds): 8.71
		        Percent of CPU this job got: 97%
		        Elapsed (wall clock) time (h:mm:ss or m:ss): 1:32.27
		        Average shared text size (kbytes): 0
		        Average unshared data size (kbytes): 0
		        Average stack size (kbytes): 0
		        Average total size (kbytes): 0
		        Maximum resident set size (kbytes): 192900
		        Average resident set size (kbytes): 0
		        Major (requiring I/O) page faults: 2
		        Minor (reclaiming a frame) page faults: 910154
		        Voluntary context switches: 2578
		        Involuntary context switches: 1564
		        Swaps: 0
		        File system inputs: 248
		        File system outputs: 288344
		        Socket messages sent: 0
		        Socket messages received: 0
		        Signals delivered: 0
		        Page size (bytes): 4096
		        Exit status: 0

[DMorsing, 2012-12-07 10:41:28]

I got around to testing this CL and it's broken in its current form.

https://codereview.appspot.com/6856126/diff/15001/src/cmd/gc/lex.c
File src/cmd/gc/lex.c (right):

https://codereview.appspot.com/6856126/diff/15001/src/cmd/gc/lex.c#newcode2019
src/cmd/gc/lex.c:2019: if(etype != Txxx && (etype != TANY || debug['A']) &&
s->def == N)
Need to flag origpkg here as well if you want to catch redeclarations of the
builtin types in the syms array.

There are also some typedefs in the 5g, 6g and 8g folders that need to be
flagged origpkg.

https://codereview.appspot.com/6856126/diff/15001/src/cmd/gc/lex.c#newcode2040
src/cmd/gc/lex.c:2040: s->origpkg = builtinpkg;
This will make redefined symbols part of the builtin package.

Move these lines into the if (s->def == N) blocks.

https://codereview.appspot.com/6856126/diff/15001/test/fixedbugs/issue4252.di...
File test/fixedbugs/issue4252.dir/a.go (right):

https://codereview.appspot.com/6856126/diff/15001/test/fixedbugs/issue4252.di...
test/fixedbugs/issue4252.dir/a.go:8: const true = 0 == 1
These consts will be turned into literal values when inlined. For a proper test,
use vars here.

[remyoudompheng, 2012-12-07 18:55:53]

Thanks for the careful review.

https://codereview.appspot.com/6856126/diff/15001/src/cmd/gc/lex.c
File src/cmd/gc/lex.c (right):

https://codereview.appspot.com/6856126/diff/15001/src/cmd/gc/lex.c#newcode2019
src/cmd/gc/lex.c:2019: if(etype != Txxx && (etype != TANY || debug['A']) &&
s->def == N)
On 2012/12/07 10:41:29, DMorsing wrote:
> Need to flag origpkg here as well if you want to catch redeclarations of the
> builtin types in the syms array.
> 
> There are also some typedefs in the 5g, 6g and 8g folders that need to be
> flagged origpkg.

Done but I don't see the typedefs in [568]g you are talking about.

https://codereview.appspot.com/6856126/diff/15001/src/cmd/gc/lex.c#newcode2040
src/cmd/gc/lex.c:2040: s->origpkg = builtinpkg;
On 2012/12/07 10:41:29, DMorsing wrote:
> This will make redefined symbols part of the builtin package.
> 
> Move these lines into the if (s->def == N) blocks.

Done.

https://codereview.appspot.com/6856126/diff/15001/test/fixedbugs/issue4252.di...
File test/fixedbugs/issue4252.dir/a.go (right):

https://codereview.appspot.com/6856126/diff/15001/test/fixedbugs/issue4252.di...
test/fixedbugs/issue4252.dir/a.go:8: const true = 0 == 1
On 2012/12/07 10:41:29, DMorsing wrote:
> These consts will be turned into literal values when inlined. For a proper
test,
> use vars here.

Done.

[DMorsing, 2012-12-07 19:06:12]

I misremembered how the code was structured. On src/cmd/gc/lex.c:2031 there's a
loop that pulls in the arch dependent types (int, uint and uintptr). Those have
to be tagged as well if there's anyone out there who is insane enough to
redefine int.

[remyoudompheng, 2012-12-07 19:14:07]

On 2012/12/07 19:06:12, DMorsing wrote:
> I misremembered how the code was structured. On src/cmd/gc/lex.c:2031 there's
a
> loop that pulls in the arch dependent types (int, uint and uintptr). Those
have
> to be tagged as well if there's anyone out there who is insane enough to
> redefine int.

Indeed, thanks.

[remyoudompheng, 2012-12-07 19:14:17]

Hello rsc@golang.org, golang-dev@googlegroups.com, mirtchovski@gmail.com,
daniel.morsing@gmail.com, dave@cheney.net, lvd@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[DMorsing, 2012-12-07 20:25:30]

LGTM.

[remyoudompheng, 2012-12-08 13:43:15]

*** Submitted as https://code.google.com/p/go/source/detail?r=6b602ab487d6 ***

cmd/gc: do not export useless private symbols.

Fixes issue 4252.

R=rsc, golang-dev, mirtchovski, daniel.morsing, dave, lvd
CC=golang-dev
https://codereview.appspot.com/6856126