FAQ
Reviewers: rsc, golang-dev_googlegroups.com,

Message:
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/


Description:
cmd/gc: do not export useless private symbols.

Fixes issue 4252.

Please review this at https://codereview.appspot.com/6856126/

Affected files:
M src/cmd/gc/export.c
M src/cmd/gc/lex.c
A test/declbuiltin.dir/a.go
A test/declbuiltin.dir/main.go
A test/declbuiltin.go

Search Discussions

  • Remyoudompheng at Dec 1, 2012 at 5:04 pm
    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


    https://codereview.appspot.com/6856126/
  • Dave Cheney at Dec 2, 2012 at 3:47 am
    I have much slower hardware at my disposal. I will do some
    measurements this afternoon.
    On Sun, Dec 2, 2012 at 4:04 AM, wrote:
    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


    https://codereview.appspot.com/6856126/
  • Mirtchovski at Dec 1, 2012 at 5:29 pm
    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.

    https://codereview.appspot.com/6856126/
  • Daniel Morsing at Dec 1, 2012 at 6:07 pm
    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?

    https://codereview.appspot.com/6856126/
  • Dave at Dec 2, 2012 at 8:41 am

    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

    https://codereview.appspot.com/6856126/
  • Lvd at Dec 4, 2012 at 12:53 am
  • Rsc at Dec 6, 2012 at 5:47 am
    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.


    https://codereview.appspot.com/6856126/
  • Remyoudompheng at Dec 6, 2012 at 7:22 am
    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.


    https://codereview.appspot.com/6856126/
  • Remyoudompheng at Dec 6, 2012 at 7:23 am
    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#newcode14
    test/declbuiltin.dir/a.go:14: func Test() {
    On 2012/12/03 22:40:30, lvd2 wrote:
    shouldnt main.go run this one?
    Done.

    http://codereview.appspot.com/6856126/
  • Dave at Dec 7, 2012 at 12:37 am
    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

    https://codereview.appspot.com/6856126/
  • Daniel Morsing at Dec 7, 2012 at 10:41 am
    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.dir/a.go
    File test/fixedbugs/issue4252.dir/a.go (right):

    https://codereview.appspot.com/6856126/diff/15001/test/fixedbugs/issue4252.dir/a.go#newcode8
    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.

    https://codereview.appspot.com/6856126/
  • Remyoudompheng at Dec 7, 2012 at 6:55 pm
    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.dir/a.go
    File test/fixedbugs/issue4252.dir/a.go (right):

    https://codereview.appspot.com/6856126/diff/15001/test/fixedbugs/issue4252.dir/a.go#newcode8
    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.

    https://codereview.appspot.com/6856126/
  • Daniel Morsing at Dec 7, 2012 at 7:06 pm
    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.

    https://codereview.appspot.com/6856126/
  • Remyoudompheng at Dec 7, 2012 at 7:14 pm

    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.

    https://codereview.appspot.com/6856126/
  • Remyoudompheng at Dec 7, 2012 at 7:14 pm
    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.


    https://codereview.appspot.com/6856126/
  • Daniel Morsing at Dec 7, 2012 at 8:25 pm
  • Remyoudompheng at Dec 8, 2012 at 1:43 pm
    *** 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


    https://codereview.appspot.com/6856126/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedDec 1, '12 at 5:01p
activeDec 8, '12 at 1:43p
posts18
users6
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase