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

remove reallocs and overallocs for s.lset in bucket.go #1718

Merged
merged 1 commit into from
Nov 5, 2019

Conversation

ppanyukov
Copy link
Contributor

@ppanyukov ppanyukov commented Nov 5, 2019

Changes

  • append to s.lset caused reallocs and overallocs
  • this is now fixed by properly setting capacity on s.lset
  • pprof shows "-316.22MB, 5.00% of 6322.82MB total" for sample data set and query I used

Verification

  • Sample data generated with thanosbench using realistic-k8s-1w-small profile
  • Query executed: count({__name__=~".+"}) by (__name__)
  • Instrumented BucketStore.Series to dump heap profile at the start and end of the function call.
  • Compared the profiles using pprof:
go tool pprof -sample_index=alloc_space -edgefraction=0 -lines -base heap-series-1-after-base.pb.gz  -http=:8080 heap-series-1-after-change.pb.gz
  • PNG attached

  • Heap profiles attached

profile001

heap-series-1-after-base.pb.gz
heap-series-1-after-change.pb.gz


Edit

For what it's worth, here is my HeapDump func I used, in case anyone wants to instrument anything. Might save you looking for pprof docs :)

func HeapDump(name string) {
	var fName = fmt.Sprintf("/Users/philip/thanos/github.com/ppanyukov/thanos-oom/scripts/%s.pb.gz", name)
	f, _ := os.Create(fName)
	defer f.Close()
	runtime.GC()
	pprof.WriteHeapProfile(f)
	fmt.Printf("WRITTEN HEAP DUMP TO %s\n", fName)
}

- append to s.lset caused reallocs and overallocs
- this is now fixed by properly setting capacity on s.lset
- pprof shows "-316.22MB, 5.00% of 6322.82MB total" for sample data set and query I used

Signed-off-by: Philip Panyukov <[email protected]>
@ppanyukov ppanyukov force-pushed the feature/bucket-realloc-fix branch from ac35ad8 to 4805283 Compare November 5, 2019 13:08
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

This is intense! =D

I wish we had some nice benchmark suite.. on my todo list. (:

I think we copy those labels not efficiently more than just here.. Thanks! small steps (:

@bwplotka bwplotka merged commit f501429 into thanos-io:master Nov 5, 2019
@GiedriusS
Copy link
Member

LGTM, thanks!

@ppanyukov ppanyukov deleted the feature/bucket-realloc-fix branch November 5, 2019 19:14
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.

3 participants