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

metadata: Fix bug where AppendToOutgoingContext could modify another context's metadata #1930

Merged
merged 2 commits into from
Mar 19, 2018

Conversation

dfawley
Copy link
Member

@dfawley dfawley commented Mar 19, 2018

Fixes #1925

For 1-10 append operations, performance is slightly worse using this method, but it is correct and ~3x faster than the old method of copying the whole map.

Before:

BenchmarkAppendToOutgoingContext/1-12                          	10000000	       197 ns/op
BenchmarkAppendToOutgoingContext/10-12                         	 1000000	      2033 ns/op
BenchmarkAppendToOutgoingContext/100-12                        	  100000	     15497 ns/op

After:

BenchmarkAppendToOutgoingContext/1-12                          	10000000	       193 ns/op
BenchmarkAppendToOutgoingContext/10-12                         	  500000	      2513 ns/op
BenchmarkAppendToOutgoingContext/100-12                        	   30000	     51658 ns/op

The old method for reference:

Benchmark_AddingMetadata_ContextManipulationApproach/1-12  	 2000000	       805 ns/op
Benchmark_AddingMetadata_ContextManipulationApproach/10-12 	  200000	      7925 ns/op
Benchmark_AddingMetadata_ContextManipulationApproach/100-12    20000	     80581 ns/op

(Note: I'm also copying kv..., because it could be passed as mySlice..., and I also added a test to demonstrate/catch that.)

@dfawley dfawley merged commit b96718f into grpc:master Mar 19, 2018
@dfawley dfawley deleted the metadata_append branch March 19, 2018 23:26
@jeanbza
Copy link
Member

jeanbza commented Mar 20, 2018

👍 good deal - thanks for the @ notification for me to look at! I need to be a lot more careful with append..

@menghanl menghanl added this to the 1.11 Release milestone Mar 27, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Sep 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants