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

performance optimisation: avoid map regrow calls #875

Merged
merged 2 commits into from
May 31, 2023
Merged

Conversation

chirayu
Copy link
Contributor

@chirayu chirayu commented May 30, 2023

No description provided.

@chirayu chirayu requested a review from hbhoyar-uber May 30, 2023 10:52
@CLAassistant
Copy link

CLAassistant commented May 30, 2023

CLA assistant check
All committers have signed the CLA.

@@ -119,9 +119,10 @@ func WithEndpointRequestHeadersField(ctx context.Context, requestHeaders map[str

// GetEndpointRequestHeadersFromCtx returns the endpoint request headers, if it exists on context
func GetEndpointRequestHeadersFromCtx(ctx context.Context) map[string]string {
requestHeaders := make(map[string]string)
var requestHeaders map[string]string
Copy link
Contributor

Choose a reason for hiding this comment

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

You should have a empty map. Currently it's null. If context doesn't have headers, then nil map will be returned and there are callers which directly add new keys to map.
Ex here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch..

@@ -205,9 +206,10 @@ func WithScopeTags(ctx context.Context, newFields map[string]string) context.Con

// GetScopeTagsFromCtx returns the tag info extracted from context.
func GetScopeTagsFromCtx(ctx context.Context) map[string]string {
tags := make(map[string]string)
var tags map[string]string
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above

@coveralls
Copy link

Coverage Status

Coverage: 69.383% (+0.004%) from 69.379% when pulling f9536ac on log-efficiency into f2aa266 on master.

runtime/context.go Show resolved Hide resolved
runtime/context.go Show resolved Hide resolved
@chirayu chirayu merged commit 0ece208 into master May 31, 2023
1 check passed
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.

None yet

4 participants