Skip to content
This repository was archived by the owner on Aug 13, 2019. It is now read-only.

Breakdown generic writeOffsetTable - #643

Merged
codesome merged 5 commits into
prometheus-junkyard:masterfrom
codesome:offset-table
Jul 10, 2019
Merged

Breakdown generic writeOffsetTable#643
codesome merged 5 commits into
prometheus-junkyard:masterfrom
codesome:offset-table

Conversation

@codesome

@codesome codesome commented Jul 2, 2019

Copy link
Copy Markdown
Contributor

This is a broken down piece of #627

With this, I am avoiding allocating slice headers for postings by having the name and value as separate fields. This saves quite some allocs.

[Don't merge until Prometheus 2.11 is out]

…and writePostingsOffsetTable

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
@cstyan

cstyan commented Jul 2, 2019

Copy link
Copy Markdown
Contributor

This saves quite some allocs.

Any sense of how much? 10%? 20%?

@codesome

codesome commented Jul 3, 2019

Copy link
Copy Markdown
Contributor Author

I don't have numbers for individual optimizations. All combined numbers is in this comment #627 (comment). I will try to get number for each optimizations.

@codesome

codesome commented Jul 5, 2019

Copy link
Copy Markdown
Contributor Author

So the gains are mostly allocs.

benchmark                                                                                 old ns/op       new ns/op       delta
BenchmarkCompaction/type=normal,blocks=2,series=1000000,samplesPerSeriesPerBlock=51-8     12824719385     12761476707     -0.49%

benchmark                                                                                 old allocs     new allocs     delta
BenchmarkCompaction/type=normal,blocks=2,series=1000000,samplesPerSeriesPerBlock=51-8     15845398       14839951       -6.35%

benchmark                                                                                 old bytes      new bytes      delta
BenchmarkCompaction/type=normal,blocks=2,series=1000000,samplesPerSeriesPerBlock=51-8     2074569456     2074402112     -0.01%

Ganesh Vernekar added 2 commits July 9, 2019 15:22
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

@gouthamve gouthamve left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, but I am not sure the minor improvement in allocs is worth this change.

Comment thread CHANGELOG.md Outdated
Comment thread index/index.go

w.postings = append(w.postings, hashEntry{
keys: []string{name, value},
w.postings = append(w.postings, postingsHashEntry{

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm just surprised that this small change reduces allocs so much.

@bwplotka bwplotka Jul 9, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Quite interesting indeed, but keep in mind that slice header is like ~16 bytes overall, so in some cases might be bigger than name or key (:

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
@codesome

codesome commented Jul 9, 2019

Copy link
Copy Markdown
Contributor Author

I am not sure the minor improvement in allocs is worth this change.

I would still like to have it. Multiple small improvements will add up to a big gain. And the absolute gain from this PR will be bigger in different scenarios (more number of label-values and series than the benchmark).

@bwplotka bwplotka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Make sesne to me. Might be small improvement but not much harm in code complexity.

@codesome
codesome merged commit 0fdd93b into prometheus-junkyard:master Jul 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants