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

Avoid unnecessary heap allocation in NewSpanStartConfig #2065

Closed
bogdandrutu opened this issue Jul 5, 2021 · 4 comments · Fixed by #2558
Closed

Avoid unnecessary heap allocation in NewSpanStartConfig #2065

bogdandrutu opened this issue Jul 5, 2021 · 4 comments · Fixed by #2558
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@bogdandrutu
Copy link
Member

Problem Statement

Right now calling NewSpanStartConfig will heap allocate SpanConfig every time is called.

Proposed Solution

Multiple solutions can be implemented:

  • Expose Apply in the Options interfaces (add another internal func to avoid extending it), so users will execute what NewSpanStartConfig does but with a var sc SpanConfig and pass the address to sc.
  • Replace NewSpanStartConfig with a ResolveSpanConfig (or other suitable name) that accepts a pointer to SpanConfig so internally SpanConfig can be stack allocated and a pointer to sc can be passed to the resolve func.

Same applies to all configs.

@bogdandrutu bogdandrutu added the enhancement New feature or request label Jul 5, 2021
@MrAlias
Copy link
Contributor

MrAlias commented Jul 8, 2021

From SIG meeting:

  • We need to have a benchmark around this. It may not be worth while to change all options if they are not created in the hot-path.
  • It might also be this is a "ghost" allocation that might be optimized away by the compiler in real-world use case. We should include a benchmark to ensure this makes sense.

@MrAlias
Copy link
Contributor

MrAlias commented Aug 19, 2021

Another proposed solution is to have the New* functions return a type itself not a pointer to the type.

@jmacd
Copy link
Contributor

jmacd commented Aug 19, 2021

I agree and support the idea. There are many examples of the proposed solution already in the metric sdk, for example

// NewMeterConfig creates a new MeterConfig and applies
// all the given options.
func NewMeterConfig(opts ...MeterOption) MeterConfig {
	var config MeterConfig
	for _, o := range opts {
		o.applyMeter(&config)
	}
	return config
}

@MrAlias
Copy link
Contributor

MrAlias commented Aug 30, 2021

Merging #2212 will allow this issue to be resolved after the stable release. All changes will be to unexported interfaces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
4 participants