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

grpc: add ability to compile with or without tracing #6954

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

hugelgupf
Copy link
Contributor

@hugelgupf hugelgupf commented Feb 1, 2024

golang.org/x/net/trace is the only dependency of gRPC Go that depends -- through html/template and text/template -- on reflect's MethodByName. Binaries with MethodByName are not eligible for Go dead code elimination.

As of protobuf v1.32.0 combined with Go 1.22, Go protobufs support compilation with dead code elimination. This commit allows users of gRPC Go to also make use of dead code elimination for smaller binary sizes.


What's the best place to insert a build test to make sure that go build -tags=grpcnotrace ./... still builds?

RELEASE NOTES:

  • *: Allow building without x/net/trace by using grpcnotrace to enable dead code elimination

Copy link

linux-foundation-easycla bot commented Feb 1, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: hugelgupf / name: Chris K (76b9bfe)

Copy link

codecov bot commented Feb 1, 2024

Codecov Report

Merging #6954 (76b9bfe) into master (84b85ba) will decrease coverage by 0.12%.
The diff coverage is 0.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6954      +/-   ##
==========================================
- Coverage   83.72%   83.61%   -0.12%     
==========================================
  Files         287      288       +1     
  Lines       30915    30921       +6     
==========================================
- Hits        25884    25854      -30     
- Misses       3969     3999      +30     
- Partials     1062     1068       +6     
Files Coverage Δ
trace.go 14.63% <ø> (ø)
stream.go 81.39% <0.00%> (ø)
server.go 81.51% <0.00%> (-0.56%) ⬇️
trace_withtrace.go 0.00% <0.00%> (ø)

... and 16 files with indirect coverage changes

@hugelgupf
Copy link
Contributor Author

I added a release notes string, the test failure looks like a flake to me. Seems ready for review :)

@dfawley dfawley self-assigned this Feb 1, 2024
@dfawley dfawley added the Type: Performance Performance improvements (CPU, network, memory, etc) label Feb 2, 2024
@dfawley dfawley added this to the 1.62 Release milestone Feb 2, 2024
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; just one administrivia item.

trace_notrace.go Outdated Show resolved Hide resolved
golang.org/x/net/trace is the only dependency of gRPC Go that depends --
through html/template and text/template -- on reflect's MethodByName.
Binaries with MethodByName are not eligible for Go dead code
elimination.

As of protobuf v1.32.0 combined with Go 1.22, Go protobufs support
compilation with dead code elimination. This commit allows users of gRPC
Go to also make use of dead code elimination for smaller binary sizes.

Signed-off-by: Chris Koch <chrisko@google.com>
@hugelgupf
Copy link
Contributor Author

Thanks @dfawley!

Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@dfawley dfawley merged commit 03e76b3 into grpc:master Feb 2, 2024
14 checks passed
@hugelgupf hugelgupf deleted the no-trace branch February 2, 2024 21:49
smira added a commit to smira/talos that referenced this pull request Feb 22, 2024
See grpc/grpc-go#6954

Before:

```
-rw-r--r-- 1 smira smira 68241300 Feb 20 20:08 _out/initramfs-amd64.xz
```

After:

```
-rw-r--r-- 1 smira smira 66376648 Feb 20 20:08 _out/initramfs-amd64.xz
```

Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>
dsseng pushed a commit to dsseng/talos that referenced this pull request Mar 7, 2024
See grpc/grpc-go#6954

Before:

```
-rw-r--r-- 1 smira smira 68241300 Feb 20 20:08 _out/initramfs-amd64.xz
```

After:

```
-rw-r--r-- 1 smira smira 66376648 Feb 20 20:08 _out/initramfs-amd64.xz
```

Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Performance Performance improvements (CPU, network, memory, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants