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.

Sign up for GitHub

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

Jump to bottom

stats/opentelemetry: Optimize slice allocations #7525

Merged
merged 2 commits into from
Sep 3, 2024
Merged

stats/opentelemetry: Optimize slice allocations #7525

merged 2 commits into from
Sep 3, 2024

Conversation

ash2k
Copy link
Contributor

@ash2k ash2k commented Aug 17, 2024 •
edited by dfawley
Loading

First commit moves attribute construction under if to only construct them when they are used.

Second commit uses WithAttributeSet() instead of WithAttributes(). This avoids copying the slice, which is not necessary for this code. See https://github.com/open-telemetry/opentelemetry-go/blob/metric/v1.28.0/metric/instrument.go#L349-L368.

Also, a bit of allocations can be removed by constructing the vararg slice one time vs when calling Record() each time.

var sink []string

func abc(vals ...string) {
	sink = vals
}

func BenchmarkFuncCall(b *testing.B) {
	b.ReportAllocs()
	b.ResetTimer()
	arg := []string{"a", "b", "c"}
	for i := 0; i < b.N; i++ {
		abc(arg...)
	}
}

The above benchmark gives me these results:

// Pass "a", "b", "c"
BenchmarkFuncCall-10    	43171615	        27.14 ns/op	      48 B/op	       1 allocs/op

// Pass arg...
BenchmarkFuncCall-10    	1000000000	         0.9530 ns/op	       0 B/op	       0 allocs/op

See https://go.dev/ref/spec#Passing_arguments_to_..._parameters.

RELEASE NOTES: None

@aranjans aranjans added this to the 1.67 Release milestone Aug 19, 2024
@purnesh42H purnesh42H requested a review from arjan-bal August 20, 2024 06:28
@arjan-bal arjan-bal self-assigned this Aug 20, 2024
@arjan-bal arjan-bal added the Type: Performance Performance improvements (CPU, network, memory, etc) label Aug 20, 2024
@arjan-bal arjan-bal changed the title Optimize stats/opentelemetry stats/opentelemetry: Optimize slice allocations Aug 20, 2024
arjan-bal
arjan-bal approved these changes Aug 20, 2024
View reviewed changes
Copy link
Contributor

@arjan-bal arjan-bal left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution!

Adding another reviewer for a second approval.

@arjan-bal arjan-bal requested a review from dfawley August 20, 2024 09:51
@arjan-bal arjan-bal assigned dfawley and unassigned arjan-bal Aug 20, 2024
@codecov Codecov
Copy link

codecov bot commented Aug 20, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.85%. Comparing base (005b092) to head (52792d2).
Report is 8 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #7525      +/-   ##
==========================================
- Coverage   81.91%   81.85%   -0.07%     
==========================================
  Files         361      361              
  Lines       27825    27825              
==========================================
- Hits        22794    22776      -18     
- Misses       3841     3853      +12     
- Partials     1190     1196       +6

see 12 files with indirect coverage changes

dfawley
dfawley reviewed Aug 28, 2024
View reviewed changes
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.

Wow, I wish otel had renamed WithAttributeSet to WithAttributes and made some other function behave the way WithAttributes behaves. That is unexpected. I might even go so far as to mark WithAttributes deprecated if I was them, to make it more clear that it's probably not what you ever want to use.

stats/opentelemetry/client_metrics.go Outdated Show resolved Hide resolved
@dfawley dfawley assigned ash2k and unassigned dfawley Aug 28, 2024
ash2k added 2 commits August 29, 2024 11:03
@ash2k
stats/opentelemetry: only construct attributes when required
e9c5307
@ash2k
stats/opentelemetry: WithAttributes() -> WithAttributeSet()
52792d2 
WithAttributes() makes a defensive copy of the passed slice, which is unnecessary overhead in this code.
@aranjans aranjans assigned dfawley and unassigned ash2k Sep 3, 2024
@dfawley dfawley merged commit 6147c81 into grpc:master Sep 3, 2024
14 checks passed
@ash2k ash2k deleted the optimize-otel branch September 3, 2024 22:56
@tbg tbg mentioned this pull request Nov 27, 2024
Draft
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arjan-bal arjan-bal arjan-bal approved these changes

@dfawley dfawley dfawley approved these changes

Assignees

@dfawley dfawley

Labels
Type: Performance Performance improvements (CPU, network, memory, etc)
Projects
None yet
Milestone
1.67 Release
Development

Successfully merging this pull request may close these issues.
4 participants
@ash2k @dfawley @arjan-bal @aranjans