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

Use a variable-size array to represent ordered labels in maps #523

Merged
merged 15 commits into from
Mar 11, 2020

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Mar 5, 2020

Rather than using a label encoder to encode a unique representation of the label set, we can use an interface{} containing an array value of the appropriate size. (This was described as "use dangerous reflection black magic" in the Metrics SIG meeting notes today.) This improve benchmarks significantly. The code has been hand-written for up to 10 labels to avoid the overhead of reflect.

Before:

goos: darwin
goarch: amd64
pkg: go.opentelemetry.io/otel/sdk/metric
BenchmarkLabels_1-8    	 4147288	       266 ns/op	      80 B/op	       2 allocs/op
BenchmarkLabels_2-8    	 2574391	       447 ns/op	      96 B/op	       2 allocs/op
BenchmarkLabels_4-8    	 1570831	       806 ns/op	     144 B/op	       2 allocs/op
BenchmarkLabels_8-8    	  833871	      1432 ns/op	     224 B/op	       2 allocs/op
BenchmarkLabels_16-8   	  445599	      2703 ns/op	     400 B/op	       2 allocs/op

After:

goos: darwin
goarch: amd64
pkg: go.opentelemetry.io/otel/sdk/metric
BenchmarkLabels_1-8    	 9424884	       124 ns/op	      96 B/op	       2 allocs/op
BenchmarkLabels_2-8    	 7479254	       163 ns/op	     144 B/op	       2 allocs/op
BenchmarkLabels_4-8    	 5067748	       236 ns/op	     240 B/op	       2 allocs/op
BenchmarkLabels_8-8    	 3274238	       359 ns/op	     432 B/op	       2 allocs/op
BenchmarkLabels_16-8   	  763551	      1377 ns/op	    1584 B/op	       3 allocs/op

@jmacd jmacd added the area:metrics Part of OpenTelemetry Metrics label Mar 5, 2020
@jmacd
Copy link
Contributor Author

jmacd commented Mar 6, 2020

Note that use of an array-valued key makes map lookups more expensive, but this is still an overall performance win.

Before:

BenchmarkInt64CounterAdd-8                             	10995777	       108 ns/op	       0 B/op	       0 allocs/op
BenchmarkInt64CounterHandleAdd-8                       	72600636	        16.4 ns/op	       0 B/op	       0 allocs/op
BenchmarkFloat64CounterAdd-8                           	10173048	       112 ns/op	       0 B/op	       0 allocs/op
BenchmarkFloat64CounterHandleAdd-8                     	63691292	        18.8 ns/op	       0 B/op	       0 allocs/op

After:

BenchmarkInt64CounterAdd-8                             	 7974315	       151 ns/op	       0 B/op	       0 allocs/op
BenchmarkInt64CounterHandleAdd-8                       	71714146	        16.6 ns/op	       0 B/op	       0 allocs/op
BenchmarkFloat64CounterAdd-8                           	 7449758	       157 ns/op	       0 B/op	       0 allocs/op
BenchmarkFloat64CounterHandleAdd-8                     	63894693	        18.6 ns/op	       0 B/op	       0 allocs/op

@jmacd
Copy link
Contributor Author

jmacd commented Mar 6, 2020

Note that this change removes the caching of encoded labels. It's possible to regain this by adding support for the label set to cache the encoding for >=1 label encoder, instead. I did it that way in OTEP 78, see here:

https://github.com/jmacd/opentelemetry-go/blob/8bed2e14df7f9f4688fbab141924bb786dc9a3a1/api/context/internal/set.go#L89

@jmacd
Copy link
Contributor Author

jmacd commented Mar 6, 2020

Another note: if the benchmark was to measure the cost of update-to-export of a single metric event, and we compare with previous "single encoding" optimization, this approach costs more overall because we're computing two encodings (one for the map and one for the export). I'm still happy with this direction because it reduces confusion about handling label encoders.

@jmacd
Copy link
Contributor Author

jmacd commented Mar 6, 2020

We can slightly optimize this code as noted here: https://github.com/open-telemetry/opentelemetry-go/pull/523/files#diff-4c3de179542bac3c5ceacd2090215160R371

As there is a forthcoming OTEP proposing to remove LabelSet, I decided to leave several potential optimizations out.

@jmacd
Copy link
Contributor Author

jmacd commented Mar 9, 2020

@bogdandrutu please 👀.

Copy link
Member

@krnowak krnowak left a comment

Choose a reason for hiding this comment

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

Mostly looks good, but have some nits and questions.

sdk/metric/sdk.go Outdated Show resolved Hide resolved
sdk/metric/sdk.go Show resolved Hide resolved
sdk/metric/sdk.go Outdated Show resolved Hide resolved
sdk/metric/sdk.go Show resolved Hide resolved
sdk/metric/sdk.go Show resolved Hide resolved
sdk/metric/sdk.go Show resolved Hide resolved
sdk/metric/sdk.go Outdated Show resolved Hide resolved
sdk/metric/correct_test.go Show resolved Hide resolved
@jmacd jmacd changed the title Use an variable-size array to represent ordered labels in maps Use a variable-size array to represent ordered labels in maps Mar 10, 2020
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Overall this looks like an improvement for me.

@jmacd jmacd mentioned this pull request Mar 11, 2020
@jmacd jmacd merged commit ae9033e into open-telemetry:master Mar 11, 2020
@jmacd jmacd deleted the jmacd/array_map branch March 11, 2020 16:11
MikeGoldsmith pushed a commit to MikeGoldsmith/opentelemetry-go that referenced this pull request Mar 13, 2020
…elemetry#523)

* Use an array key to label encoding in the SDK

* Comment

* Precommit

* Comment

* Comment

* Feedback from krnowak

* Do not overwrite the Key

* Add the value test requested

* Add a comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants