fix: avoid memory allocations when loading symbolizer from a flushed chunk - #17953
Conversation
| // Reset resets all the data and makes the symbolizer ready for reuse | ||
| func (s *symbolizer) Reset() { | ||
| s.mtx.Lock() | ||
| defer s.mtx.Unlock() | ||
|
|
||
| s.symbolsMap = map[string]uint32{} | ||
| s.labels = s.labels[:0] | ||
| s.size = 0 | ||
| s.compressedSize = 0 | ||
| s.normalizedNames = map[uint32]string{} | ||
| } |
There was a problem hiding this comment.
surprised to see this removed? do we not use it?
Fine with removing it just wanted to double check
There was a problem hiding this comment.
Yeah, the symbolizer is created per chunk, and since we do not reuse chunk references, we do not reuse symbolizer either. We will have to use a pool to reuse symbolizer, but we will have to see how much it would help since the volume of structured metadata won't be balanced.
| } | ||
| e.entries = append(displaced[0].(*nsEntries).entries, nsEntry{line, hb.symbolizer.Add(structuredMetadata)}) | ||
| var symbols symbols | ||
| if !structuredMetadata.IsEmpty() { |
There was a problem hiding this comment.
not sure I understand this change?
There was a problem hiding this comment.
It was to fix some broken tests after my changes. However, it seems I can remove that check since the symbolizer is never nil in non-test code. I have fixed the broken tests to pass a non-nil symbolizer.
slim-bean
left a comment
slim-bean
left a comment
There was a problem hiding this comment.
LGTM but I did have a couple questions though they don't have to block merging.
Also it would be nice to add a few comments that the symbolsMap is only necessary when the chunk is being written to, maybe explain what it's purpose is, this is what confused me when I made the last optimization.
![grafana-delivery-bot[bot]](https://cdn.statically.io/img/github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
2 participants
What this PR does / why we need it:
When loading a symbolizer from a flushed chunk, we unnecessarily allocate a map that holds entries only to dedupe the data while writing to a chunk. This PR fixes the issue by removing those unwanted allocations and rejecting any writes when the symbolizer is loaded from a flushed chunk.
Checklist