Skip to content

Copy label values to avoid unsafe memory use #2006

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

Closed
wants to merge 2 commits into from

Conversation

bboreham
Copy link
Contributor

@bboreham bboreham commented Jan 20, 2020

This is an alternative solution to #2000 (and #2004) where we do more copying on the error path but need fewer comments to avoid mistakes.

Benchmarks show this alternative is significantly more expensive on the error path:
Before #2000

BenchmarkIngesterPush/encoding=DoubleDelta	   20165041 ns/op  2590736 B/op	   12441 allocs/op
BenchmarkIngesterPushErrors/encoding=DoubleDelta   26534724 ns/op  5270153 B/op	   85194 allocs/op

After #2000

BenchmarkIngesterPush/encoding=DoubleDelta	   20305149 ns/op  2591191 B/op	   12444 allocs/op
BenchmarkIngesterPushErrors/encoding=DoubleDelta   27959445 ns/op  5276005 B/op	   85189 allocs/op

This branch:

BenchmarkIngesterPush/encoding=DoubleDelta	   20074468 ns/op  2591331 B/op	   12598 allocs/op
BenchmarkIngesterPushErrors/encoding=DoubleDelta   35415601 ns/op 11072246 B/op	   95055 allocs/op

@bboreham bboreham changed the title Copy labels push errors Copy label values to avoid unsafe memory use Jan 20, 2020
@pracucci
Copy link
Contributor

@bboreham Do you mind rebasing? This stuff has changed in the WAL PR #1103 which has just been merged 😞

@bboreham
Copy link
Contributor Author

I think the benchmarks show that we don't want to do this so have updated #2004.

@bboreham bboreham closed this Jan 23, 2020
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.

2 participants