Implement SelectMergeTree via the model.Tree#4790
Draft
simonswine wants to merge 10 commits intomainfrom
Draft
Conversation
simonswine
changed the title
SelectMergeTree via the query
simonswine
changed the title
SelectMergeTree via the querySelectMergeTree via the model.Tree
simonswine
force-pushed
the
20260122_generic-tree
branch
2 times, most recently
from
232757f to
6b0b68b
Compare
simonswine
force-pushed
the
20260122_generic-tree
branch
3 times, most recently
from
0aa934b to
7f557ec
Compare
simonswine
commented
Feb 17, 2026
| @@ -0,0 +1,614 @@ | |||
| package symdb | |||
Contributor
Author
There was a problem hiding this comment.
This is the most complex part of this PR, worth starting to look at it @aleks-p
Contributor
There was a problem hiding this comment.
@simonswine looks good to me overall, I think we can move it forward once we address the few TODOs
aleks-p
reviewed
Feb 20, 2026
pkg/phlaredb/symdb/symbol_merger.go
Outdated
| h.sl = slices.Grow(h.sl, size) | ||
| h.hashes = slices.Grow(h.hashes, size) | ||
| // grow map if still empty | ||
| if len(h.m) == 0 { |
Contributor
There was a problem hiding this comment.
nit, this is perhaps never hit, we always call add when creating new mergers
pkg/phlaredb/symdb/symbol_merger.go
Outdated
| type equalityFn[A any] func(A, A) bool | ||
|
|
||
| type hashedSlice[A any] struct { | ||
| merger *SymbolMerger |
Contributor
There was a problem hiding this comment.
nit, it doesn't seem like we use this
| @@ -0,0 +1,614 @@ | |||
| package symdb | |||
Contributor
There was a problem hiding this comment.
@simonswine looks good to me overall, I think we can move it forward once we address the few TODOs
- [x] TODO fix "other" - [ ] Make sure truncation works on max symbols rather than max nodes - [x] Consistently number IDs starting from 1 - [ ] Share logic of getting pprof from tree for profilecli
hashedSlice.add() previously panicked on 64-bit xxhash collisions. Replace the panic with linear probing on the map key so collisions are handled gracefully: the probe hash is incremented until an empty slot is found, while the original content hash is always stored in hashes[] for use by parent-level hash computation. Add TestSymbolMerger_HashCollision to verify that two distinct values sharing the same hash receive distinct indices, that original hashes are preserved, and that deduplication still works for both values.
simonswine
force-pushed
the
20260122_generic-tree
branch
from
bb7c19c to
948c5fe
Compare
Contributor
Author
|
I really should have enabled the integration tests a longer time ago, basically the symbolizer isn't able to work correctly with this feature. I will have to spent some more time on this. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
We currently use a completely separate query path for SelectMergeStacktraces vs. SelectMergePprof calls. This PR is a first step in unifying the two paths, by implementing a model.Tree based merge.
This refactors model.Tree to be generic over its node name type, supporting both string-based function names (FuntionName) and integer location references (LocationRefName). This enables the query-backend path to build flamegraph trees using location refs directly, deferring string resolution until the final merge step.
Most complexity is in the SymbolMerger in symdb that deduplicates and merges symbols (strings, mappings, functions, locations) across profile blocks using content hashing (xxhash), with linear probing to gracefully handle hash collisions.
After merging this, the old path is still active by default, there is a per-tenant override, gating the tree-based SelectMergeProfile code path. Also adds test coverage for the query backend, frontend query path, and the symbol merger.
Note: The integration tests after this PR will only target the tree based variant.