pkg/query/flamegraph_arrow: Trim using breadth-first-search #3722

metalmatze · 2023-08-30T18:40:32Z

This adds trimming based on breadth-first-search using dictionary compaction.

It passes the tests locally now. 😌 🎉

Some TODOs are left either to fix within this PR or as follow-ups.

Remove the sorting of label rows from production code (required)
Either update Arrow or use a fork for GH-37465: [Go] Add Value method to BooleanBuilder apache/arrow#37459 (required)
Fix all the memory leaks with the CheckedAllocator (seems like there might have been leaks before)
Potentially upstream the compactDictionary at some point later.

metalmatze · 2023-08-30T18:41:40Z

pkg/query/flamegraph_arrow.go

+	rootsRowKeys := make([]uint64, 0, len(fb.rootsRow))
+	for k := range fb.rootsRow {
+		rootsRowKeys = append(rootsRowKeys, k)
+	}
+	sort.Slice(rootsRowKeys, func(i, j int) bool { return rootsRowKeys[i] < rootsRowKeys[j] })


This is bad, it should be done in the tests.

metalmatze · 2023-08-30T18:42:11Z

pkg/query/flamegraph_arrow.go

@@ -1036,7 +1078,7 @@ func (fb *flamegraphBuilder) Release() {
 	fb.builderFunctionFilenameDictUnifier.Release()

 	fb.builderChildren.Release()
-	fb.builderChildrenValues.Release()
+	// fb.builderChildrenValues.Release()


This shouldn't be here. Slipped as part of me trying to fix the allocations.

metalmatze · 2023-08-30T18:42:38Z

pkg/query/flamegraph_arrow.go

+	_, span := tracer.Start(ctx, "trim")
+	defer span.End()
+
+	releasers := make([]releasable, 0, 3)


3 is incorrect here. Figure out the real number.

metalmatze · 2023-08-30T18:43:27Z

pkg/query/flamegraph_arrow.go

+	// initialize the queue with the root rows' children. It usually has the most amount of children.
+	trimmingQueue := queue{elements: make([]trimmingElement, 0, len(fb.children[0]))}
+	trimmingQueue.push(trimmingElement{row: 0})
+
+	// keep processing new elements until the queue is empty
+	for trimmingQueue.len() > 0 {


This is the little goodie of this PR 😌 We should really quickly walk the flame graph to find the rows to trim now 😎

metalmatze · 2023-08-30T18:43:52Z

pkg/query/flamegraph_arrow.go

+	trimmedMappingBuildIDIndicesArray := trimmedMappingBuildIDIndices.NewArray()
+	releasers = append(releasers, trimmedMappingBuildIDIndicesArray)
+	fb.mappingBuildID, err = compactDictionary(fb.pool, array.NewDictionaryArray(
+		&arrow.DictionaryType{IndexType: trimmedMappingBuildIDIndicesArray.DataType(), ValueType: fb.mappingBuildID.DataType()},
+		trimmedMappingBuildIDIndicesArray,
+		fb.mappingBuildID.Dictionary(),
+	))


These could be great with helper functions.

metalmatze · 2023-08-30T18:45:06Z

pkg/query/flamegraph_arrow_test.go

+			// level 0 - root
+			{MappingStart: 0, MappingLimit: 0, MappingOffset: 0, MappingFile: array.NullValueStr, MappingBuildID: array.NullValueStr, LocationAddress: 0, LocationFolded: false, LocationLine: 0, FunctionStartLine: 0, FunctionName: `(null)`, FunctionSystemName: array.NullValueStr, FunctionFilename: array.NullValueStr, Cumulative: 10, Labels: nil, Children: []uint32{1, 2, 3}}, // 0
+			// level 1
+			{MappingStart: 1, MappingLimit: 1, MappingOffset: 0x1234, MappingFile: "a", MappingBuildID: "aID", LocationAddress: 0xa1, LocationFolded: false, LocationLine: 1, FunctionStartLine: 1, FunctionName: "1", FunctionSystemName: "1", FunctionFilename: "1", Cumulative: 3, Labels: nil, Children: []uint32{4}},                                                                                                          // 1
+			{MappingStart: 0, MappingLimit: 0, MappingOffset: 0, MappingFile: array.NullValueStr, MappingBuildID: array.NullValueStr, LocationAddress: 0, LocationFolded: false, LocationLine: 0, FunctionStartLine: 0, FunctionName: `(null)`, FunctionSystemName: array.NullValueStr, FunctionFilename: array.NullValueStr, Cumulative: 4, Labels: map[string]string{"goroutine": "2"}, Children: []uint32{5}, LabelsOnly: true}, // 2
+			{MappingStart: 0, MappingLimit: 0, MappingOffset: 0, MappingFile: array.NullValueStr, MappingBuildID: array.NullValueStr, LocationAddress: 0, LocationFolded: false, LocationLine: 0, FunctionStartLine: 0, FunctionName: `(null)`, FunctionSystemName: array.NullValueStr, FunctionFilename: array.NullValueStr, Cumulative: 3, Labels: map[string]string{"goroutine": "1"}, Children: []uint32{6}, LabelsOnly: true}, // 3
+			// level 2
+			{MappingStart: 1, MappingLimit: 1, MappingOffset: 0x1234, MappingFile: "a", MappingBuildID: "aID", LocationAddress: 0xa2, LocationFolded: false, LocationLine: 2, FunctionStartLine: 2, FunctionName: "2", FunctionSystemName: "2", FunctionFilename: "2", Cumulative: 3, Labels: nil, Children: []uint32{7}},                                 // 4
+			{MappingStart: 1, MappingLimit: 1, MappingOffset: 0x1234, MappingFile: "a", MappingBuildID: "aID", LocationAddress: 0xa1, LocationFolded: false, LocationLine: 1, FunctionStartLine: 1, FunctionName: "1", FunctionSystemName: "1", FunctionFilename: "1", Cumulative: 4, Labels: map[string]string{"goroutine": "2"}, Children: []uint32{8}}, // 6
+			{MappingStart: 1, MappingLimit: 1, MappingOffset: 0x1234, MappingFile: "a", MappingBuildID: "aID", LocationAddress: 0xa1, LocationFolded: false, LocationLine: 1, FunctionStartLine: 1, FunctionName: "1", FunctionSystemName: "1", FunctionFilename: "1", Cumulative: 3, Labels: map[string]string{"goroutine": "1"}, Children: []uint32{9}}, // 5
+			// level 3
+			{MappingStart: 1, MappingLimit: 1, MappingOffset: 0x1234, MappingFile: "a", MappingBuildID: "aID", LocationAddress: 0xa3, LocationFolded: false, LocationLine: 3, FunctionStartLine: 3, FunctionName: "3", FunctionSystemName: "3", FunctionFilename: "3", Cumulative: 3, Labels: nil, Children: []uint32{10}},                                 // 7
+			{MappingStart: 1, MappingLimit: 1, MappingOffset: 0x1234, MappingFile: "a", MappingBuildID: "aID", LocationAddress: 0xa2, LocationFolded: false, LocationLine: 2, FunctionStartLine: 2, FunctionName: "2", FunctionSystemName: "2", FunctionFilename: "2", Cumulative: 4, Labels: map[string]string{"goroutine": "2"}, Children: []uint32{11}}, // 9
+			{MappingStart: 1, MappingLimit: 1, MappingOffset: 0x1234, MappingFile: "a", MappingBuildID: "aID", LocationAddress: 0xa2, LocationFolded: false, LocationLine: 2, FunctionStartLine: 2, FunctionName: "2", FunctionSystemName: "2", FunctionFilename: "2", Cumulative: 3, Labels: map[string]string{"goroutine": "1"}, Children: []uint32{12}}, // 8
+			// level 4
+			{MappingStart: 1, MappingLimit: 1, MappingOffset: 0x1234, MappingFile: "a", MappingBuildID: "aID", LocationAddress: 0xa4, LocationFolded: false, LocationLine: 4, FunctionStartLine: 4, FunctionName: "4", FunctionSystemName: "4", FunctionFilename: "4", Cumulative: 3, Labels: nil, Children: nil},                                          // 10
+			{MappingStart: 1, MappingLimit: 1, MappingOffset: 0x1234, MappingFile: "a", MappingBuildID: "aID", LocationAddress: 0xa3, LocationFolded: false, LocationLine: 3, FunctionStartLine: 3, FunctionName: "3", FunctionSystemName: "3", FunctionFilename: "3", Cumulative: 4, Labels: map[string]string{"goroutine": "2"}, Children: []uint32{13}}, // 12
+			{MappingStart: 1, MappingLimit: 1, MappingOffset: 0x1234, MappingFile: "a", MappingBuildID: "aID", LocationAddress: 0xa3, LocationFolded: false, LocationLine: 3, FunctionStartLine: 3, FunctionName: "3", FunctionSystemName: "3", FunctionFilename: "3", Cumulative: 1, Labels: map[string]string{"goroutine": "1"}, Children: []uint32{14}}, // 11
+			// level 5
+			{MappingStart: 1, MappingLimit: 1, MappingOffset: 0x1234, MappingFile: "a", MappingBuildID: "aID", LocationAddress: 0xa5, LocationFolded: false, LocationLine: 5, FunctionStartLine: 5, FunctionName: "5", FunctionSystemName: "5", FunctionFilename: "5", Cumulative: 4, Labels: map[string]string{"goroutine": "2"}, Children: nil}, // 13
+			{MappingStart: 1, MappingLimit: 1, MappingOffset: 0x1234, MappingFile: "a", MappingBuildID: "aID", LocationAddress: 0xa5, LocationFolded: false, LocationLine: 5, FunctionStartLine: 5, FunctionName: "5", FunctionSystemName: "5", FunctionFilename: "5", Cumulative: 1, Labels: map[string]string{"goroutine": "1"}, Children: nil}, // 14


These rows have changed order now.
Whereas before this was depth-first order due to using breadth-first order search in the trimming the order changed to be on a per level basis.

metalmatze · 2023-08-30T18:45:21Z

pkg/query/flamegraph_arrow_test.go

@@ -462,7 +469,7 @@ func TestGenerateFlamegraphArrowWithInlined(t *testing.T) {

 	ctx := context.Background()
 	mem := memory.NewCheckedAllocator(memory.DefaultAllocator)
-	defer mem.AssertSize(t, 0)
+	// defer mem.AssertSize(t, 0)


Enable this when leaks are fixed.

alwaysmeticulous · 2023-08-30T18:46:36Z

✅ Meticulous spotted zero visual differences across 245 screens tested: view results.

_{Last updated for commit 759eabe. This comment will update as new commits are pushed.}

pkg/query/flamegraph_arrow.go

thorfour · 2023-08-30T21:39:15Z

My initial set of fixes can be found at the branch fix-flamegraph-arrow-trim-bfs-leaks They got kind of extensive and there's still a too many releases error from them, so that's why I didn't direct merge here.

pkg/query: Add string dictionary compaction pkg/query: Trim arrow flame graph and compact dictionaries pkg/query/flamegraph_arrow: Make trimming children work pkg/query/flamegraph_arrow: Release arrays after passing them to dictionary pkg/query/flamegraph_arrow: Implement trimming for label columns pkg/query/flamegraph_arrow: Fix dictionary values to binary instead of string

Co-authored-by: Thor <[email protected]>

Also the mapping and function dictionaries don't leak here. It must be somewhere before that.

metalmatze · 2023-08-31T17:47:24Z

I messed this branch up force-pushing not able to concentrate today.
Thanks to @thorfour for carrying the torch further in #3730

…3730) * pkg/query/flamegraph_arrow: Trim using breadth-first-search pkg/query: Add string dictionary compaction pkg/query: Trim arrow flame graph and compact dictionaries pkg/query/flamegraph_arrow: Make trimming children work pkg/query/flamegraph_arrow: Release arrays after passing them to dictionary pkg/query/flamegraph_arrow: Implement trimming for label columns pkg/query/flamegraph_arrow: Fix dictionary values to binary instead of string * actually release * fix dictionary building If you ran with `-tags assert` the tests would panic * release records before reassignment * fix TestGenerateFlamegraphArrowWithInlined allocations * mod: replace with binary builder branch * go mod: remove replace * remove sorting * assert size on remaining of tests * right size the releasers slice * pkg/query/flamegraph_arrow: Fix panic when children are nil Additionally, address some linting issues. --------- Co-authored-by: Matthias Loibl <[email protected]>

metalmatze requested a review from a team as a code owner August 30, 2023 18:40

metalmatze mentioned this pull request Aug 30, 2023

pkg/query/flamegraph_arrow: Trim children if below trim threshold #3448

Closed

metalmatze commented Aug 30, 2023

View reviewed changes

vercel bot deployed to Preview – parca-ui August 30, 2023 18:46 View deployment

vercel bot deployed to Preview – parca-ui August 30, 2023 21:34 View deployment

thorfour reviewed Aug 30, 2023

View reviewed changes

pkg/query/flamegraph_arrow.go Outdated Show resolved Hide resolved

thorfour force-pushed the flamegraph-arrow-trim-bfs branch from c2cff02 to f9c98f7 Compare August 31, 2023 02:03

thorfour requested a review from a team as a code owner August 31, 2023 02:03

vercel bot deployed to Preview – parca-ui August 31, 2023 02:18 View deployment

metalmatze and others added 2 commits August 31, 2023 11:05

pkg/query/flamegraph_arrow: Fix memory leaks after rebasing

e996149

Co-authored-by: Thor <[email protected]>

metalmatze force-pushed the flamegraph-arrow-trim-bfs branch from f9c98f7 to e996149 Compare August 31, 2023 10:37

vercel bot deployed to Preview – parca-ui August 31, 2023 10:43 View deployment

pkg/query/flamegraph_arrow: Fix more memory leaks after rebasing

94141b7

vercel bot deployed to Preview – parca-ui August 31, 2023 11:15 View deployment

pkg/query/flamegraph_arrow: Fix label dictionaries

759eabe

Also the mapping and function dictionaries don't leak here. It must be somewhere before that.

vercel bot deployed to Preview – parca-ui August 31, 2023 11:41 View deployment

thorfour mentioned this pull request Aug 31, 2023

pkg/query/flamegraph_arrow: Trim using breadth-first-search #3722 #3730

Merged

4 tasks

metalmatze closed this Aug 31, 2023

metalmatze deleted the flamegraph-arrow-trim-bfs branch October 26, 2023 14:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/query/flamegraph_arrow: Trim using breadth-first-search #3722

pkg/query/flamegraph_arrow: Trim using breadth-first-search #3722

metalmatze commented Aug 30, 2023 •

edited by brancz

Loading

metalmatze Aug 30, 2023

metalmatze Aug 30, 2023

metalmatze Aug 30, 2023

metalmatze Aug 30, 2023

metalmatze Aug 30, 2023

metalmatze Aug 30, 2023

metalmatze Aug 30, 2023

alwaysmeticulous bot commented Aug 30, 2023 •

edited

Loading

thorfour commented Aug 30, 2023 •

edited

Loading

metalmatze commented Aug 31, 2023

pkg/query/flamegraph_arrow: Trim using breadth-first-search #3722

pkg/query/flamegraph_arrow: Trim using breadth-first-search #3722

Conversation

metalmatze commented Aug 30, 2023 • edited by brancz Loading

metalmatze Aug 30, 2023

Choose a reason for hiding this comment

metalmatze Aug 30, 2023

Choose a reason for hiding this comment

metalmatze Aug 30, 2023

Choose a reason for hiding this comment

metalmatze Aug 30, 2023

Choose a reason for hiding this comment

metalmatze Aug 30, 2023

Choose a reason for hiding this comment

metalmatze Aug 30, 2023

Choose a reason for hiding this comment

metalmatze Aug 30, 2023

Choose a reason for hiding this comment

alwaysmeticulous bot commented Aug 30, 2023 • edited Loading

thorfour commented Aug 30, 2023 • edited Loading

metalmatze commented Aug 31, 2023

metalmatze commented Aug 30, 2023 •

edited by brancz

Loading

alwaysmeticulous bot commented Aug 30, 2023 •

edited

Loading

thorfour commented Aug 30, 2023 •

edited

Loading