From 28aa26f30a466e5f7638407973c3998f5304f188 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 16 Apr 2025 20:22:39 +0200 Subject: [PATCH] Store Pipe objects by value in slice of Pipes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This saves some memory at the cost of a slight performance increase (I suppose reallocting the slice when adding new Pipes is slightly more expensive now). Performance of the BenchmarkRenderCommitGraph benchmark is 130μs before, 175μs after. I'm guessing this is still acceptable. --- pkg/gui/presentation/commits.go | 4 +-- pkg/gui/presentation/graph/graph.go | 36 +++++++++---------- pkg/gui/presentation/graph/graph_test.go | 46 ++++++++++++------------ 3 files changed, 43 insertions(+), 43 deletions(-) diff --git a/pkg/gui/presentation/commits.go b/pkg/gui/presentation/commits.go index 7e6e0e838..0e1530415 100644 --- a/pkg/gui/presentation/commits.go +++ b/pkg/gui/presentation/commits.go @@ -28,7 +28,7 @@ type pipeSetCacheKey struct { } var ( - pipeSetCache = make(map[pipeSetCacheKey][][]*graph.Pipe) + pipeSetCache = make(map[pipeSetCacheKey][][]graph.Pipe) mutex deadlock.Mutex ) @@ -245,7 +245,7 @@ func indexOfFirstNonTODOCommit(commits []*models.Commit) int { return 0 } -func loadPipesets(commits []*models.Commit) [][]*graph.Pipe { +func loadPipesets(commits []*models.Commit) [][]graph.Pipe { // given that our cache key is a commit hash and a commit count, it's very important that we don't actually try to render pipes // when dealing with things like filtered commits. cacheKey := pipeSetCacheKey{ diff --git a/pkg/gui/presentation/graph/graph.go b/pkg/gui/presentation/graph/graph.go index 5c5240fdd..b769e6db5 100644 --- a/pkg/gui/presentation/graph/graph.go +++ b/pkg/gui/presentation/graph/graph.go @@ -56,20 +56,20 @@ func RenderCommitGraph(commits []*models.Commit, selectedCommitHashPtr *string, return lines } -func GetPipeSets(commits []*models.Commit, getStyle func(c *models.Commit) style.TextStyle) [][]*Pipe { +func GetPipeSets(commits []*models.Commit, getStyle func(c *models.Commit) style.TextStyle) [][]Pipe { if len(commits) == 0 { return nil } - pipes := []*Pipe{{fromPos: 0, toPos: 0, fromHash: &StartCommitHash, toHash: commits[0].HashPtr(), kind: STARTS, style: style.FgDefault}} + pipes := []Pipe{{fromPos: 0, toPos: 0, fromHash: &StartCommitHash, toHash: commits[0].HashPtr(), kind: STARTS, style: style.FgDefault}} - return lo.Map(commits, func(commit *models.Commit, _ int) []*Pipe { + return lo.Map(commits, func(commit *models.Commit, _ int) []Pipe { pipes = getNextPipes(pipes, commit, getStyle) return pipes }) } -func RenderAux(pipeSets [][]*Pipe, commits []*models.Commit, selectedCommitHashPtr *string) []string { +func RenderAux(pipeSets [][]Pipe, commits []*models.Commit, selectedCommitHashPtr *string) []string { maxProcs := runtime.GOMAXPROCS(0) // splitting up the rendering of the graph into multiple goroutines allows us to render the graph in parallel @@ -106,7 +106,7 @@ func RenderAux(pipeSets [][]*Pipe, commits []*models.Commit, selectedCommitHashP return lo.Flatten(chunks) } -func getNextPipes(prevPipes []*Pipe, commit *models.Commit, getStyle func(c *models.Commit) style.TextStyle) []*Pipe { +func getNextPipes(prevPipes []Pipe, commit *models.Commit, getStyle func(c *models.Commit) style.TextStyle) []Pipe { maxPos := 0 for _, pipe := range prevPipes { if pipe.toPos > maxPos { @@ -116,11 +116,11 @@ func getNextPipes(prevPipes []*Pipe, commit *models.Commit, getStyle func(c *mod // a pipe that terminated in the previous line has no bearing on the current line // so we'll filter those out - currentPipes := lo.Filter(prevPipes, func(pipe *Pipe, _ int) bool { + currentPipes := lo.Filter(prevPipes, func(pipe Pipe, _ int) bool { return pipe.kind != TERMINATES }) - newPipes := make([]*Pipe, 0, len(currentPipes)+len(commit.ParentPtrs())) + newPipes := make([]Pipe, 0, len(currentPipes)+len(commit.ParentPtrs())) // start by assuming that we've got a brand new commit not related to any preceding commit. // (this only happens when we're doing `git log --all`). These will be tacked onto the far end. pos := maxPos + 1 @@ -143,7 +143,7 @@ func getNextPipes(prevPipes []*Pipe, commit *models.Commit, getStyle func(c *mod } else { toHash = commit.ParentPtrs()[0] } - newPipes = append(newPipes, &Pipe{ + newPipes = append(newPipes, Pipe{ fromPos: pos, toPos: pos, fromHash: commit.HashPtr(), @@ -195,7 +195,7 @@ func getNextPipes(prevPipes []*Pipe, commit *models.Commit, getStyle func(c *mod for _, pipe := range currentPipes { if equalHashes(pipe.toHash, commit.HashPtr()) { // terminating here - newPipes = append(newPipes, &Pipe{ + newPipes = append(newPipes, Pipe{ fromPos: pipe.toPos, toPos: pos, fromHash: pipe.fromHash, @@ -207,7 +207,7 @@ func getNextPipes(prevPipes []*Pipe, commit *models.Commit, getStyle func(c *mod } else if pipe.toPos < pos { // continuing here availablePos := getNextAvailablePosForContinuingPipe() - newPipes = append(newPipes, &Pipe{ + newPipes = append(newPipes, Pipe{ fromPos: pipe.toPos, toPos: availablePos, fromHash: pipe.fromHash, @@ -223,7 +223,7 @@ func getNextPipes(prevPipes []*Pipe, commit *models.Commit, getStyle func(c *mod for _, parent := range commit.ParentPtrs()[1:] { availablePos := getNextAvailablePosForNewPipe() // need to act as if continuing pipes are going to continue on the same line. - newPipes = append(newPipes, &Pipe{ + newPipes = append(newPipes, Pipe{ fromPos: pos, toPos: availablePos, fromHash: commit.HashPtr(), @@ -247,7 +247,7 @@ func getNextPipes(prevPipes []*Pipe, commit *models.Commit, getStyle func(c *mod last = i } } - newPipes = append(newPipes, &Pipe{ + newPipes = append(newPipes, Pipe{ fromPos: pipe.toPos, toPos: last, fromHash: pipe.fromHash, @@ -260,7 +260,7 @@ func getNextPipes(prevPipes []*Pipe, commit *models.Commit, getStyle func(c *mod } // not efficient but doing it for now: sorting my pipes by toPos, then by kind - slices.SortFunc(newPipes, func(a, b *Pipe) int { + slices.SortFunc(newPipes, func(a, b Pipe) int { if a.toPos == b.toPos { return cmp.Compare(a.kind, b.kind) } @@ -271,7 +271,7 @@ func getNextPipes(prevPipes []*Pipe, commit *models.Commit, getStyle func(c *mod } func renderPipeSet( - pipes []*Pipe, + pipes []Pipe, selectedCommitHashPtr *string, prevCommit *models.Commit, ) string { @@ -330,19 +330,19 @@ func renderPipeSet( // so we have our commit pos again, now it's time to build the cells. // we'll handle the one that's sourced from our selected commit last so that it can override the other cells. - selectedPipes, nonSelectedPipes := utils.Partition(pipes, func(pipe *Pipe) bool { + selectedPipes, nonSelectedPipes := utils.Partition(pipes, func(pipe Pipe) bool { return highlight && equalHashes(pipe.fromHash, selectedCommitHashPtr) }) for _, pipe := range nonSelectedPipes { if pipe.kind == STARTS { - renderPipe(pipe, pipe.style, true) + renderPipe(&pipe, pipe.style, true) } } for _, pipe := range nonSelectedPipes { if pipe.kind != STARTS && !(pipe.kind == TERMINATES && pipe.fromPos == commitPos && pipe.toPos == commitPos) { - renderPipe(pipe, pipe.style, false) + renderPipe(&pipe, pipe.style, false) } } @@ -352,7 +352,7 @@ func renderPipeSet( } } for _, pipe := range selectedPipes { - renderPipe(pipe, highlightStyle, true) + renderPipe(&pipe, highlightStyle, true) if pipe.toPos == commitPos { cells[pipe.toPos].setStyle(highlightStyle) } diff --git a/pkg/gui/presentation/graph/graph_test.go b/pkg/gui/presentation/graph/graph_test.go index 25d529bbc..56ae2cb95 100644 --- a/pkg/gui/presentation/graph/graph_test.go +++ b/pkg/gui/presentation/graph/graph_test.go @@ -261,7 +261,7 @@ func TestRenderPipeSet(t *testing.T) { tests := []struct { name string - pipes []*Pipe + pipes []Pipe commit *models.Commit prevCommit *models.Commit expectedStr string @@ -269,7 +269,7 @@ func TestRenderPipeSet(t *testing.T) { }{ { name: "single cell", - pipes: []*Pipe{ + pipes: []Pipe{ {fromPos: 0, toPos: 0, fromHash: pool("a"), toHash: pool("b"), kind: TERMINATES, style: cyan}, {fromPos: 0, toPos: 0, fromHash: pool("b"), toHash: pool("c"), kind: STARTS, style: green}, }, @@ -279,7 +279,7 @@ func TestRenderPipeSet(t *testing.T) { }, { name: "single cell, selected", - pipes: []*Pipe{ + pipes: []Pipe{ {fromPos: 0, toPos: 0, fromHash: pool("a"), toHash: pool("selected"), kind: TERMINATES, style: cyan}, {fromPos: 0, toPos: 0, fromHash: pool("selected"), toHash: pool("c"), kind: STARTS, style: green}, }, @@ -289,7 +289,7 @@ func TestRenderPipeSet(t *testing.T) { }, { name: "terminating hook and starting hook, selected", - pipes: []*Pipe{ + pipes: []Pipe{ {fromPos: 0, toPos: 0, fromHash: pool("a"), toHash: pool("selected"), kind: TERMINATES, style: cyan}, {fromPos: 1, toPos: 0, fromHash: pool("c"), toHash: pool("selected"), kind: TERMINATES, style: yellow}, {fromPos: 0, toPos: 0, fromHash: pool("selected"), toHash: pool("d"), kind: STARTS, style: green}, @@ -303,7 +303,7 @@ func TestRenderPipeSet(t *testing.T) { }, { name: "terminating hook and starting hook, prioritise the terminating one", - pipes: []*Pipe{ + pipes: []Pipe{ {fromPos: 0, toPos: 0, fromHash: pool("a"), toHash: pool("b"), kind: TERMINATES, style: red}, {fromPos: 1, toPos: 0, fromHash: pool("c"), toHash: pool("b"), kind: TERMINATES, style: magenta}, {fromPos: 0, toPos: 0, fromHash: pool("b"), toHash: pool("d"), kind: STARTS, style: green}, @@ -317,7 +317,7 @@ func TestRenderPipeSet(t *testing.T) { }, { name: "starting and terminating pipe sharing some space", - pipes: []*Pipe{ + pipes: []Pipe{ {fromPos: 0, toPos: 0, fromHash: pool("a1"), toHash: pool("a2"), kind: TERMINATES, style: red}, {fromPos: 0, toPos: 0, fromHash: pool("a2"), toHash: pool("a3"), kind: STARTS, style: yellow}, {fromPos: 1, toPos: 1, fromHash: pool("b1"), toHash: pool("b2"), kind: CONTINUES, style: magenta}, @@ -332,7 +332,7 @@ func TestRenderPipeSet(t *testing.T) { }, { name: "starting and terminating pipe sharing some space, with selection", - pipes: []*Pipe{ + pipes: []Pipe{ {fromPos: 0, toPos: 0, fromHash: pool("a1"), toHash: pool("selected"), kind: TERMINATES, style: red}, {fromPos: 0, toPos: 0, fromHash: pool("selected"), toHash: pool("a3"), kind: STARTS, style: yellow}, {fromPos: 1, toPos: 1, fromHash: pool("b1"), toHash: pool("b2"), kind: CONTINUES, style: magenta}, @@ -347,7 +347,7 @@ func TestRenderPipeSet(t *testing.T) { }, { name: "many terminating pipes", - pipes: []*Pipe{ + pipes: []Pipe{ {fromPos: 0, toPos: 0, fromHash: pool("a1"), toHash: pool("a2"), kind: TERMINATES, style: red}, {fromPos: 0, toPos: 0, fromHash: pool("a2"), toHash: pool("a3"), kind: STARTS, style: yellow}, {fromPos: 1, toPos: 0, fromHash: pool("b1"), toHash: pool("a2"), kind: TERMINATES, style: magenta}, @@ -361,7 +361,7 @@ func TestRenderPipeSet(t *testing.T) { }, { name: "starting pipe passing through", - pipes: []*Pipe{ + pipes: []Pipe{ {fromPos: 0, toPos: 0, fromHash: pool("a1"), toHash: pool("a2"), kind: TERMINATES, style: red}, {fromPos: 0, toPos: 0, fromHash: pool("a2"), toHash: pool("a3"), kind: STARTS, style: yellow}, {fromPos: 0, toPos: 3, fromHash: pool("a2"), toHash: pool("d3"), kind: STARTS, style: yellow}, @@ -376,7 +376,7 @@ func TestRenderPipeSet(t *testing.T) { }, { name: "starting and terminating path crossing continuing path", - pipes: []*Pipe{ + pipes: []Pipe{ {fromPos: 0, toPos: 0, fromHash: pool("a1"), toHash: pool("a2"), kind: TERMINATES, style: red}, {fromPos: 0, toPos: 0, fromHash: pool("a2"), toHash: pool("a3"), kind: STARTS, style: yellow}, {fromPos: 0, toPos: 1, fromHash: pool("a2"), toHash: pool("b3"), kind: STARTS, style: yellow}, @@ -391,7 +391,7 @@ func TestRenderPipeSet(t *testing.T) { }, { name: "another clash of starting and terminating paths", - pipes: []*Pipe{ + pipes: []Pipe{ {fromPos: 0, toPos: 0, fromHash: pool("a1"), toHash: pool("a2"), kind: TERMINATES, style: red}, {fromPos: 0, toPos: 0, fromHash: pool("a2"), toHash: pool("a3"), kind: STARTS, style: yellow}, {fromPos: 0, toPos: 1, fromHash: pool("a2"), toHash: pool("b3"), kind: STARTS, style: yellow}, @@ -406,7 +406,7 @@ func TestRenderPipeSet(t *testing.T) { }, { name: "commit whose previous commit is selected", - pipes: []*Pipe{ + pipes: []Pipe{ {fromPos: 0, toPos: 0, fromHash: pool("selected"), toHash: pool("a2"), kind: TERMINATES, style: red}, {fromPos: 0, toPos: 0, fromHash: pool("a2"), toHash: pool("a3"), kind: STARTS, style: yellow}, }, @@ -418,7 +418,7 @@ func TestRenderPipeSet(t *testing.T) { }, { name: "commit whose previous commit is selected and is a merge commit", - pipes: []*Pipe{ + pipes: []Pipe{ {fromPos: 0, toPos: 0, fromHash: pool("selected"), toHash: pool("a2"), kind: TERMINATES, style: red}, {fromPos: 1, toPos: 1, fromHash: pool("selected"), toHash: pool("b3"), kind: CONTINUES, style: red}, }, @@ -430,7 +430,7 @@ func TestRenderPipeSet(t *testing.T) { }, { name: "commit whose previous commit is selected and is a merge commit, with continuing pipe inbetween", - pipes: []*Pipe{ + pipes: []Pipe{ {fromPos: 0, toPos: 0, fromHash: pool("selected"), toHash: pool("a2"), kind: TERMINATES, style: red}, {fromPos: 1, toPos: 1, fromHash: pool("z1"), toHash: pool("z3"), kind: CONTINUES, style: green}, {fromPos: 2, toPos: 2, fromHash: pool("selected"), toHash: pool("b3"), kind: CONTINUES, style: red}, @@ -443,7 +443,7 @@ func TestRenderPipeSet(t *testing.T) { }, { name: "when previous commit is selected, not a merge commit, and spawns a continuing pipe", - pipes: []*Pipe{ + pipes: []Pipe{ {fromPos: 0, toPos: 0, fromHash: pool("a1"), toHash: pool("a2"), kind: TERMINATES, style: red}, {fromPos: 0, toPos: 0, fromHash: pool("a2"), toHash: pool("a3"), kind: STARTS, style: green}, {fromPos: 0, toPos: 1, fromHash: pool("a2"), toHash: pool("b3"), kind: STARTS, style: green}, @@ -486,25 +486,25 @@ func TestGetNextPipes(t *testing.T) { pool := func(s string) *string { return hashPool.Add(s) } tests := []struct { - prevPipes []*Pipe + prevPipes []Pipe commit *models.Commit - expected []*Pipe + expected []Pipe }{ { - prevPipes: []*Pipe{ + prevPipes: []Pipe{ {fromPos: 0, toPos: 0, fromHash: pool("a"), toHash: pool("b"), kind: STARTS, style: style.FgDefault}, }, commit: models.NewCommit(hashPool, models.NewCommitOpts{ Hash: "b", Parents: []string{"c"}, }), - expected: []*Pipe{ + expected: []Pipe{ {fromPos: 0, toPos: 0, fromHash: pool("a"), toHash: pool("b"), kind: TERMINATES, style: style.FgDefault}, {fromPos: 0, toPos: 0, fromHash: pool("b"), toHash: pool("c"), kind: STARTS, style: style.FgDefault}, }, }, { - prevPipes: []*Pipe{ + prevPipes: []Pipe{ {fromPos: 0, toPos: 0, fromHash: pool("a"), toHash: pool("b"), kind: TERMINATES, style: style.FgDefault}, {fromPos: 0, toPos: 0, fromHash: pool("b"), toHash: pool("c"), kind: STARTS, style: style.FgDefault}, {fromPos: 0, toPos: 1, fromHash: pool("b"), toHash: pool("d"), kind: STARTS, style: style.FgDefault}, @@ -513,21 +513,21 @@ func TestGetNextPipes(t *testing.T) { Hash: "d", Parents: []string{"e"}, }), - expected: []*Pipe{ + expected: []Pipe{ {fromPos: 0, toPos: 0, fromHash: pool("b"), toHash: pool("c"), kind: CONTINUES, style: style.FgDefault}, {fromPos: 1, toPos: 1, fromHash: pool("b"), toHash: pool("d"), kind: TERMINATES, style: style.FgDefault}, {fromPos: 1, toPos: 1, fromHash: pool("d"), toHash: pool("e"), kind: STARTS, style: style.FgDefault}, }, }, { - prevPipes: []*Pipe{ + prevPipes: []Pipe{ {fromPos: 0, toPos: 0, fromHash: pool("a"), toHash: pool("root"), kind: TERMINATES, style: style.FgDefault}, }, commit: models.NewCommit(hashPool, models.NewCommitOpts{ Hash: "root", Parents: []string{}, }), - expected: []*Pipe{ + expected: []Pipe{ {fromPos: 1, toPos: 1, fromHash: pool("root"), toHash: pool(models.EmptyTreeCommitHash), kind: STARTS, style: style.FgDefault}, }, },