Use bytetstring instead of string for performance and switch proto libraries. #7
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mattdurham
commented
Dec 11, 2024
| } | ||
| wr.Timeseries = wr.Timeseries[:len(series)] | ||
| func createWriteRequest(series []*types.TimeSeriesBinary, externalLabels map[string]string, data *proto.Buffer) ([]byte, error) { | ||
| wr := &prompb.WriteRequest{Timeseries: make([]prompb.TimeSeries, len(series))} |
There was a problem hiding this comment.
Moving this to inside the func had no impact on CPU but saved memory.
thampiotr
approved these changes
Dec 11, 2024
| @@ -48,6 +48,72 @@ func TestLabels(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func BenchmarkFill(b *testing.B) { | |||
There was a problem hiding this comment.
Roughly 20% better if memory serves me and my notes are accurate.
Comment on lines
+165
to
+167
| // Assume roughly each series has 10 labels, we do this because at very large mappings growing the map took up to 5% of cpu time. | ||
| // By pre allocating it that disappeared. | ||
| strMapToIndex := make(map[string]uint32, len(s.series)*10) |
There was a problem hiding this comment.
Interesting. Kinda surprised it had so big impact. Maybe we can track the average size of this and autotune it at runtime?
There was a problem hiding this comment.
I was also surprised. This trades memory for cpu, ie this map is bigger than we actually need in most cases since we store only the unique strings.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The gogo library though deprecated has better performance than googles and is what prometheus uses. The bytestring is safe as long as the underlying buffer is not reused, which it is not.
PS: the branch name is because originally I tried interning the strings with the new unique package but it ended up not saving memory and increased cpu significantly. If/when we make the whole prometheus pipeline interned then this will need to change.