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

Add MD.Clone function #177

Merged
merged 1 commit into from
Dec 28, 2024
Merged

Add MD.Clone function #177

merged 1 commit into from
Dec 28, 2024

Conversation

djdongjin
Copy link
Member

@djdongjin djdongjin commented Dec 26, 2024

MD is not thread-safe so sometimes we need to make a copy before modification, e.g. in containerd,

https://github.com/containerd/containerd/blob/c3efa0cb339ffc2d6c9f03d7fb4e5e6577d0194b/pkg/namespaces/ttrpc.go#L30-L36

This PR adds a MD.Clone function, similar to golang's http.Header.Clone (the code is copied there). Also add a benchmark that shows golang's implementation is better than a simple copy approach (both memory&latency):

$ go test -bench='Benchmark.*Clone' -benchmem
goos: linux
goarch: amd64
pkg: github.com/containerd/ttrpc
cpu: Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz
BenchmarkMetadataClone/size=5-4          2663775               494.2 ns/op            80 B/op          1 allocs/op
BenchmarkMetadataClone/size=10-4          707372              1608 ns/op             871 B/op          2 allocs/op
BenchmarkMetadataClone/size=20-4          371546              2829 ns/op            1785 B/op          2 allocs/op
BenchmarkMetadataClone/size=50-4          194526              5800 ns/op            4436 B/op          3 allocs/op
BenchmarkSimpleMetadataClone/size=5-4            1809780               679.0 ns/op            80 B/op          5 allocs/op
BenchmarkSimpleMetadataClone/size=10-4            525523              2128 ns/op             871 B/op         11 allocs/op
BenchmarkSimpleMetadataClone/size=20-4            272677              4355 ns/op            2583 B/op         22 allocs/op
BenchmarkSimpleMetadataClone/size=50-4            100525             10794 ns/op            6798 B/op         55 allocs/op
PASS
ok      github.com/containerd/ttrpc     11.859s

metadata.go Outdated
Comment on lines 82 to 83
// Preserve nil values. ReverseProxy distinguishes
// between nil and zero-length header values.
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think we shouldn't copy this comment verbatim from the HTTP implementation. Any mention of a ReverseProxy is a potential source for confusion, as ttrpc which is typically used for communication between two process on the same physical machine, using a unix domain socket as the transport.

Maybe we should just leave

if vv == nil { // Preserve nil values.

here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion @klihub . SGTM, updated the PR.

@djdongjin djdongjin requested a review from klihub December 27, 2024 15:58
@djdongjin djdongjin force-pushed the metadata-clone branch 2 times, most recently from b80dcd2 to 1a82100 Compare December 27, 2024 16:11
Signed-off-by: Jin Dong <[email protected]>
@klihub klihub requested a review from fuweid December 27, 2024 17:41
@fuweid fuweid merged commit 3b8c8b7 into containerd:main Dec 28, 2024
11 checks passed
@djdongjin
Copy link
Member Author

Hi @fuweid thanks for approving/merging this PR! I wonder if we could do a ttrpc release recently (ideally also include this dependency upgrade PR #178 but it's optional).

This is mainly to fix a concurrent map write panic in otelttrpc (containerd/otelttrpc#2), which is reported in containerd/containerd#11138)

thanks

@fuweid
Copy link
Member

fuweid commented Dec 30, 2024

ping @djdongjin https://github.com/containerd/ttrpc/releases/tag/v1.2.7 this is new tag.

@djdongjin
Copy link
Member Author

ping @djdongjin https://github.com/containerd/ttrpc/releases/tag/v1.2.7 this is new tag.

Great, thanks for the quick release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants