only include quic-trace when the quictrace build flag is set

[marten-seemann]

Fixes #2797.

I'm not ready to drop quic-trace yet, but it turns out to be really easy to hide behind a build tag.

With quic-trace:

 ❯ go build -tags quictrace -o server example/main.go &&  go build -tags quictrace -o client ./example/client/main.go && du -sh client server
 14M	client
 15M	server

Without quic-trace:

❯ go build -o server example/main.go &&  go build -o client ./example/client/main.go && du -sh client server
8.8M	client
 13M	server

Not sure why the difference is so much bigger for the client. @egonelbre any ideas?

[codecov]

Codecov Report

Merging #2799 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2799   +/-   ##
=======================================
  Coverage   86.02%   86.02%           
=======================================
  Files         133      133           
  Lines       12063    12063           
=======================================
  Hits        10376    10376           
  Misses       1355     1355           
  Partials      332      332           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93e733a...3611382. Read the comment docs.

[egonelbre]

One of the issues with using a build tag is that it means it cannot be used in libraries. I.e. everyone using a library would need to modify their setup and similarly when building.

There are few other ways. One is to serialize pb manually. As an example pprof uses this approach https://golang.org/src/runtime/pprof/protomem.go.

The other is to separate the implementation and interface into separate packages. So, if you don't import quictrace as a user, you won't import all the protobuf things.

I'll take a look at the size difference thing...

[marten-seemann]

One of the issues with using a build tag is that it means it cannot be used in libraries. I.e. everyone using a library would need to modify their setup and similarly when building.

That's a feature here, not a bug. quic-trace is not intended to be used in production anyway. If you want logging / tracing, we have a better interface for that now (logging.Tracer).

[egonelbre]

That's a feature here, not a bug. quic-trace is not intended to be used in production anyway.

Ah, sorry I misunderstood... I was thinking you would need to use quictrace to disable it :D. Cleary I haven't had my morning coffee yet.

[egonelbre]

So based on preliminary research:

s:\deps\quic-go\example>goda list -std quictrace=1(./client) - ./client
github.com/golang/protobuf/proto
github.com/lucas-clemente/quic-go/quictrace/pb
go/token
google.golang.org/protobuf/encoding/prototext
google.golang.org/protobuf/encoding/protowire
google.golang.org/protobuf/internal/descfmt
google.golang.org/protobuf/internal/descopts
google.golang.org/protobuf/internal/detrand
google.golang.org/protobuf/internal/encoding/defval
google.golang.org/protobuf/internal/encoding/messageset
google.golang.org/protobuf/internal/encoding/tag
google.golang.org/protobuf/internal/encoding/text
google.golang.org/protobuf/internal/errors
google.golang.org/protobuf/internal/fieldnum
google.golang.org/protobuf/internal/fieldsort
google.golang.org/protobuf/internal/filedesc
google.golang.org/protobuf/internal/filetype
google.golang.org/protobuf/internal/flags
google.golang.org/protobuf/internal/genname
google.golang.org/protobuf/internal/impl
google.golang.org/protobuf/internal/mapsort
google.golang.org/protobuf/internal/pragma
google.golang.org/protobuf/internal/set
google.golang.org/protobuf/internal/strs
google.golang.org/protobuf/internal/version
google.golang.org/protobuf/proto
google.golang.org/protobuf/reflect/protoreflect
google.golang.org/protobuf/reflect/protoregistry
google.golang.org/protobuf/runtime/protoiface
google.golang.org/protobuf/runtime/protoimpl
hash/fnv
regexp
regexp/syntax

s:\deps\quic-go\example>goda list -std quictrace=1(.) - .
github.com/golang/protobuf/proto
github.com/lucas-clemente/quic-go/quictrace/pb
go/token
google.golang.org/protobuf/encoding/prototext
google.golang.org/protobuf/encoding/protowire
google.golang.org/protobuf/internal/descfmt
google.golang.org/protobuf/internal/descopts
google.golang.org/protobuf/internal/detrand
google.golang.org/protobuf/internal/encoding/defval
google.golang.org/protobuf/internal/encoding/messageset
google.golang.org/protobuf/internal/encoding/tag
google.golang.org/protobuf/internal/encoding/text
google.golang.org/protobuf/internal/errors
google.golang.org/protobuf/internal/fieldnum
google.golang.org/protobuf/internal/fieldsort
google.golang.org/protobuf/internal/filedesc
google.golang.org/protobuf/internal/filetype
google.golang.org/protobuf/internal/flags
google.golang.org/protobuf/internal/genname
google.golang.org/protobuf/internal/impl
google.golang.org/protobuf/internal/mapsort
google.golang.org/protobuf/internal/pragma
google.golang.org/protobuf/internal/set
google.golang.org/protobuf/internal/strs
google.golang.org/protobuf/internal/version
google.golang.org/protobuf/proto
google.golang.org/protobuf/reflect/protoreflect
google.golang.org/protobuf/reflect/protoregistry
google.golang.org/protobuf/runtime/protoiface
google.golang.org/protobuf/runtime/protoimpl
hash/fnv

The only package difference seems to be regexp, which shouldn't cause that large difference. This suggests there's something getting elided for compilation.

Few interesting differences in compiled sizes between the client and server without quictrace:

Client:
              1646175 R pclntab
    310819     305267 /net/http [syms 776]
    327957     327957 /crypto/tls [syms 560]
     39436      39436 /github.com/francoispqt/gojay [syms 77]
    279216     279216 /github.com/marten-seemann/qtls-go1-15 [syms 485]

Server:
              2542357 R pclntab
    368152     344144 /net/http [syms 1083]
    253372     253372 /crypto/tls [syms 473]
    265514     265514 /github.com/francoispqt/gojay [syms 1179]
    348441     348441 /github.com/marten-seemann/qtls-go1-15 [syms 573]

I need to add some new features to goda to get some better info.

[marten-seemann]

I'm not familiar with goda, so I'll need a bit of help with the output. Can you confirm that protobuf is not included in the binary when compiling without the quictrace tag?

The difference in the size of gojay is surprising. There shouldn't be any significant differences between client and server, as both of them use that library for qlog support.

[egonelbre]

goda -std quictrace=1(./client) - ./client

Says, take the package graph (including std packages) of ./client with tag quictrace enabled and subtract the package graph of ./client (without quictrace). Which essentially means, list all the things that were removed.

So, yes, for both server and client protobuf is removed from dependency graph.

[egonelbre]

My main guess is that something is calling reflect.Value.call, which causes code eliding to go into conservative mode. https://blog.golang.org/go1.7-binary-size. I've yet to find the exact code that does it.

[marten-seemann]

We do use reflection in the frame parser, for generating an error message: https://github.com/lucas-clemente/quic-go/blob/93e733a860ec78de71bf5f4b0912f644ccdcab0a/internal/wire/frame_parser.go#L97-L99
I wasn't aware that there are any negative effects of this, we could certainly find another solution here, if this is really blowing up the binary size.

Also, reflections are used for sanity checks in the qtls package:
https://github.com/lucas-clemente/quic-go/blob/93e733a860ec78de71bf5f4b0912f644ccdcab0a/internal/qtls/structs_equal.go#L7-L21
It would be a lot harder to replace those...

[egonelbre]

Those shouldn't affect it. AFAIR only things like https://golang.org/pkg/reflect/#Value.Call cause it.

[marten-seemann]

Missed another one in https://github.com/lucas-clemente/quic-go/blob/93e733a860ec78de71bf5f4b0912f644ccdcab0a/session.go#L1116-L1118
But that's also not a Value.Call.

For reference, the ack command to search for it is the following:

ack --ignore-dir=mocks --ignore-file="match:/(.*)_test.go$" --go reflect

[egonelbre]

So, yeah these two methods:

reflect.Type.Method
reflect.Value.MethodByName

https://golang.org/src/cmd/link/internal/ld/deadcode.go#L227

[marten-seemann]

We're not using those, so we should be safe.

[egonelbre]

Clearly, something is using them in the server code. See output from:

go tool nm client.exe | grep reflect.Value.call
go tool nm server.exe | grep reflect.Value.call

[marten-seemann]

Interesting. I guess it's something outside of quic-go then, and it's not qtls. gojay seems to use reflect at a few places (although it claims not to use reflection), but not Value.Call either.

Any idea how to find the culprit without manually grepping through all dependencies?

[egonelbre]

Yeah, trying to find a way to find it. It seems there should be a symbol attribute for all the funcs https://github.com/golang/go/blob/master/src/cmd/internal/obj/link.go#L508 for it, however, I'm still trying to find a way to read it from the binary.

[egonelbre]

Ok, figured it out. The server imports net/http/pprof, which in turn imports html/template, which imports text/template, which contains a call to MethodByName.

[marten-seemann]

That's an artifact of the example server then, and doesn't affect normal usage of the library. Thanks for digging into this!

[egonelbre]

I made a corresponding issue golang/go#41569. I think it should be possible to make net/http/pprof better.