7b0921e fix(renderer): address review findings · vev

M pkg/renderer/benchmark_test.go +56 -62


@@ -82,69 +82,63 @@ func BenchmarkRendererFragmentedDamage(b *testing.B) {
82	82		b.ReportMetric(float64(outBytes)/float64(b.N), "outbytes/op")
83	83		}
84	84
85		-	func BenchmarkRendererIncrementalOneCellPrepareCommit(b *testing.B) {
86		-	frame := NewFrame(120, 40)
87		-	markBenchmarkFrame(&frame)
88		-	r := New(Capabilities{})
89		-	initial, err := r.Prepare(frame, []Damage{FullRedraw()}, false)
90		-	if err != nil {
91		-	b.Fatal(err)
92		-	}
93		-	initial.Commit()
94		-	damage := []Damage{{Kind: DamageText, X: 60, Y: 20, Width: 1, Height: 1}}
95		-
96		-	b.ReportAllocs()
97		-	var outBytes int64
98		-	changed := false
99		-	for b.Loop() {
100		-	changed = !changed
101		-	cell := Cell{Rune: 'X', Style: DefaultStyle()}
102		-	if changed {
103		-	cell.Rune = 'Y'
104		-	}
105		-	frame.Set(60, 20, cell)
106		-	prepared, err := r.Prepare(frame, damage, false)
107		-	if err != nil {
108		-	b.Fatal(err)
109		-	}
110		-	if len(prepared.Bytes()) == 0 {
111		-	b.Fatal("expected renderer output")
112		-	}
113		-	outBytes += int64(len(prepared.Bytes()))
114		-	prepared.Commit()
115		-	}
116		-	b.ReportMetric(float64(outBytes)/float64(b.N), "outbytes/op")
117		-	}
118		-
119		-	func BenchmarkRendererIncrementalOneCellDraw(b *testing.B) {
120		-	frame := NewFrame(120, 40)
121		-	markBenchmarkFrame(&frame)
122		-	r := New(Capabilities{})
123		-	if _, err := r.Draw(frame, []Damage{FullRedraw()}); err != nil {
124		-	b.Fatal(err)
125		-	}
126		-	damage := []Damage{{Kind: DamageText, X: 60, Y: 20, Width: 1, Height: 1}}
127		-
128		-	b.ReportAllocs()
129		-	var outBytes int64
130		-	changed := false
131		-	for b.Loop() {
132		-	changed = !changed
133		-	cell := Cell{Rune: 'X', Style: DefaultStyle()}
134		-	if changed {
135		-	cell.Rune = 'Y'
136		-	}
137		-	frame.Set(60, 20, cell)
138		-	out, err := r.Draw(frame, damage)
139		-	if err != nil {
140		-	b.Fatal(err)
141		-	}
142		-	if len(out) == 0 {
143		-	b.Fatal("expected renderer output")
144		-	}
145		-	outBytes += int64(len(out))
	85	+	func BenchmarkRendererIncrementalOneCell(b *testing.B) {
	86	+	for _, tt := range []struct {
	87	+	name string
	88	+	prepareCommit bool
	89	+	}{
	90	+	{name: "PrepareCommit", prepareCommit: true},
	91	+	{name: "Draw"},
	92	+	} {
	93	+	b.Run(tt.name, func(b *testing.B) {
	94	+	frame := NewFrame(120, 40)
	95	+	markBenchmarkFrame(&frame)
	96	+	r := New(Capabilities{})
	97	+	if tt.prepareCommit {
	98	+	initial, err := r.Prepare(frame, []Damage{FullRedraw()}, false)
	99	+	if err != nil {
	100	+	b.Fatal(err)
	101	+	}
	102	+	initial.Commit()
	103	+	} else if _, err := r.Draw(frame, []Damage{FullRedraw()}); err != nil {
	104	+	b.Fatal(err)
	105	+	}
	106	+	damage := []Damage{{Kind: DamageText, X: 60, Y: 20, Width: 1, Height: 1}}
	107	+
	108	+	b.ReportAllocs()
	109	+	var outBytes int64
	110	+	changed := false
	111	+	for b.Loop() {
	112	+	changed = !changed
	113	+	cell := Cell{Rune: 'X', Style: DefaultStyle()}
	114	+	if changed {
	115	+	cell.Rune = 'Y'
	116	+	}
	117	+	frame.Set(60, 20, cell)
	118	+	if tt.prepareCommit {
	119	+	prepared, err := r.Prepare(frame, damage, false)
	120	+	if err != nil {
	121	+	b.Fatal(err)
	122	+	}
	123	+	if len(prepared.Bytes()) == 0 {
	124	+	b.Fatal("expected renderer output")
	125	+	}
	126	+	outBytes += int64(len(prepared.Bytes()))
	127	+	prepared.Commit()
	128	+	continue
	129	+	}
	130	+	out, err := r.Draw(frame, damage)
	131	+	if err != nil {
	132	+	b.Fatal(err)
	133	+	}
	134	+	if len(out) == 0 {
	135	+	b.Fatal("expected renderer output")
	136	+	}
	137	+	outBytes += int64(len(out))
	138	+	}
	139	+	b.ReportMetric(float64(outBytes)/float64(b.N), "outbytes/op")
	140	+	})
146	141		}
147		-	b.ReportMetric(float64(outBytes)/float64(b.N), "outbytes/op")
148	142		}
149	143
150	144		func BenchmarkRendererIncrementalNoBytePrepareCommit(b *testing.B) {


@@ -82,69 +82,63 @@ func BenchmarkRendererFragmentedDamage(b *testing.B) {
82		b.ReportMetric(float64(outBytes)/float64(b.N), "outbytes/op")	82		b.ReportMetric(float64(outBytes)/float64(b.N), "outbytes/op")
83		}	83		}
84			84
85	-	func BenchmarkRendererIncrementalOneCellPrepareCommit(b *testing.B) {	85	+	func BenchmarkRendererIncrementalOneCell(b *testing.B) {
86	-	frame := NewFrame(120, 40)	86	+	for _, tt := range []struct {
87	-	markBenchmarkFrame(&frame)	87	+	name string
88	-	r := New(Capabilities{})	88	+	prepareCommit bool
89	-	initial, err := r.Prepare(frame, []Damage{FullRedraw()}, false)	89	+	}{
90	-	if err != nil {	90	+	{name: "PrepareCommit", prepareCommit: true},
91	-	b.Fatal(err)	91	+	{name: "Draw"},
92	-	}	92	+	} {
93	-	initial.Commit()	93	+	b.Run(tt.name, func(b *testing.B) {
94	-	damage := []Damage{{Kind: DamageText, X: 60, Y: 20, Width: 1, Height: 1}}	94	+	frame := NewFrame(120, 40)
95	-		95	+	markBenchmarkFrame(&frame)
96	-	b.ReportAllocs()	96	+	r := New(Capabilities{})
97	-	var outBytes int64	97	+	if tt.prepareCommit {
98	-	changed := false	98	+	initial, err := r.Prepare(frame, []Damage{FullRedraw()}, false)
99	-	for b.Loop() {	99	+	if err != nil {
100	-	changed = !changed	100	+	b.Fatal(err)
101	-	cell := Cell{Rune: 'X', Style: DefaultStyle()}	101	+	}
102	-	if changed {	102	+	initial.Commit()
103	-	cell.Rune = 'Y'	103	+	} else if _, err := r.Draw(frame, []Damage{FullRedraw()}); err != nil {
104	-	}	104	+	b.Fatal(err)
105	-	frame.Set(60, 20, cell)	105	+	}
106	-	prepared, err := r.Prepare(frame, damage, false)	106	+	damage := []Damage{{Kind: DamageText, X: 60, Y: 20, Width: 1, Height: 1}}
107	-	if err != nil {	107	+
108	-	b.Fatal(err)	108	+	b.ReportAllocs()
109	-	}	109	+	var outBytes int64
110	-	if len(prepared.Bytes()) == 0 {	110	+	changed := false
111	-	b.Fatal("expected renderer output")	111	+	for b.Loop() {
112	-	}	112	+	changed = !changed
113	-	outBytes += int64(len(prepared.Bytes()))	113	+	cell := Cell{Rune: 'X', Style: DefaultStyle()}
114	-	prepared.Commit()	114	+	if changed {
115	-	}	115	+	cell.Rune = 'Y'
116	-	b.ReportMetric(float64(outBytes)/float64(b.N), "outbytes/op")	116	+	}
117	-	}	117	+	frame.Set(60, 20, cell)
118	-		118	+	if tt.prepareCommit {
119	-	func BenchmarkRendererIncrementalOneCellDraw(b *testing.B) {	119	+	prepared, err := r.Prepare(frame, damage, false)
120	-	frame := NewFrame(120, 40)	120	+	if err != nil {
121	-	markBenchmarkFrame(&frame)	121	+	b.Fatal(err)
122	-	r := New(Capabilities{})	122	+	}
123	-	if _, err := r.Draw(frame, []Damage{FullRedraw()}); err != nil {	123	+	if len(prepared.Bytes()) == 0 {
124	-	b.Fatal(err)	124	+	b.Fatal("expected renderer output")
125	-	}	125	+	}
126	-	damage := []Damage{{Kind: DamageText, X: 60, Y: 20, Width: 1, Height: 1}}	126	+	outBytes += int64(len(prepared.Bytes()))
127	-		127	+	prepared.Commit()
128	-	b.ReportAllocs()	128	+	continue
129	-	var outBytes int64	129	+	}
130	-	changed := false	130	+	out, err := r.Draw(frame, damage)
131	-	for b.Loop() {	131	+	if err != nil {
132	-	changed = !changed	132	+	b.Fatal(err)
133	-	cell := Cell{Rune: 'X', Style: DefaultStyle()}	133	+	}
134	-	if changed {	134	+	if len(out) == 0 {
135	-	cell.Rune = 'Y'	135	+	b.Fatal("expected renderer output")
136	-	}	136	+	}
137	-	frame.Set(60, 20, cell)	137	+	outBytes += int64(len(out))
138	-	out, err := r.Draw(frame, damage)	138	+	}
139	-	if err != nil {	139	+	b.ReportMetric(float64(outBytes)/float64(b.N), "outbytes/op")
140	-	b.Fatal(err)	140	+	})
141	-	}
142	-	if len(out) == 0 {
143	-	b.Fatal("expected renderer output")
144	-	}
145	-	outBytes += int64(len(out))
146		}	141		}
147	-	b.ReportMetric(float64(outBytes)/float64(b.N), "outbytes/op")
148		}	142		}
149			143
150		func BenchmarkRendererIncrementalNoBytePrepareCommit(b *testing.B) {	144		func BenchmarkRendererIncrementalNoBytePrepareCommit(b *testing.B) {

M pkg/renderer/damage_plan.go +16 -12


@@ -19,18 +19,7 @@ func buildDamagePlan(frame Frame, damage []Damage, skip *Damage) ([]Span, bool)
19	19		if d.Kind != DamageText && d.Kind != DamageClear {
20	20		return nil, false
21	21		}
22		-	x, y, width, height, ok := clampRect(frame, d.X, d.Y, d.Width, d.Height)
23		-	if !ok {
24		-	return nil, false
25		-	}
26		-	if height > maxPlannedDamageSpans {
27		-	return nil, true
28		-	}
29		-	spans := make([]Span, height)
30		-	for row := range height {
31		-	spans[row] = Span{Y: y + row, X: x, Width: width}
32		-	}
33		-	return spans, false
	22	+	return buildSingleDamageSpans(frame, d)
34	23		}
35	24
36	25		spans := make([]Span, 0)

@@ -63,6 +52,21 @@ func buildDamagePlan(frame Frame, damage []Damage, skip *Damage) ([]Span, bool)
63	52		return mergeDamageSpans(spans), false
64	53		}
65	54
	55	+	func buildSingleDamageSpans(frame Frame, d Damage) ([]Span, bool) {
	56	+	x, y, width, height, ok := clampRect(frame, d.X, d.Y, d.Width, d.Height)
	57	+	if !ok {
	58	+	return nil, false
	59	+	}
	60	+	if height > maxPlannedDamageSpans {
	61	+	return nil, true
	62	+	}
	63	+	spans := make([]Span, height)
	64	+	for row := range height {
	65	+	spans[row] = Span{Y: y + row, X: x, Width: width}
	66	+	}
	67	+	return spans, false
	68	+	}
	69	+
66	70		func mergeDamageSpans(spans []Span) []Span {
67	71		if len(spans) < 2 {
68	72		return spans

 		if d.Kind != DamageText && d.Kind != DamageClear {
 			return nil, false
 		}
-		x, y, width, height, ok := clampRect(frame, d.X, d.Y, d.Width, d.Height)
+		return buildSingleDamageSpans(frame, d)
-		if !ok {
-			return nil, false
-		}
-		if height > maxPlannedDamageSpans {
-			return nil, true
-		}
-		spans := make([]Span, height)
-		for row := range height {
-			spans[row] = Span{Y: y + row, X: x, Width: width}
-		}
-		return spans, false
 	}
 	spans := make([]Span, 0)
 	return mergeDamageSpans(spans), false
 }
+func buildSingleDamageSpans(frame Frame, d Damage) ([]Span, bool) {
+	x, y, width, height, ok := clampRect(frame, d.X, d.Y, d.Width, d.Height)
+	if !ok {
+		return nil, false
+	}
+	if height > maxPlannedDamageSpans {
+		return nil, true
+	}
+	spans := make([]Span, height)
+	for row := range height {
+		spans[row] = Span{Y: y + row, X: x, Width: width}
+	}
+	return spans, false
+}
 func mergeDamageSpans(spans []Span) []Span {
 	if len(spans) < 2 {
 		return spans

M pkg/renderer/delta_plan.go +3 -10


@@ -89,19 +89,12 @@ func PlanDelta(frame Frame, damage []Damage, committed Frame, reset bool) (Delta
89	89		}
90	90
91	91		func planSingleDamage(frame Frame, d Damage) DeltaPlan {
92		-	x, y, width, height, ok := clampRect(frame, d.X, d.Y, d.Width, d.Height)
93		-	if !ok {
94		-	return DeltaPlan{}
95		-	}
96		-	if height > maxPlannedDamageSpans {
	92	+	spans, full := buildSingleDamageSpans(frame, d)
	93	+	if full {
97	94		return DeltaPlan{Snapshot: true}
98	95		}
99		-	spans := make([]Span, height)
100		-	for row := range height {
101		-	spans[row] = Span{Y: y + row, X: x, Width: width}
102		-	}
103	96		plan := DeltaPlan{Spans: spans}
104		-	if (frame.Height == 1 \|\| len(spans) > 1) && deltaCostsSnapshot(frame, spans, false) {
	97	+	if deltaCostsSnapshot(frame, spans, false) {
105	98		plan.Snapshot = true
106	99		}
107	100		return plan

 }
 func planSingleDamage(frame Frame, d Damage) DeltaPlan {
-	x, y, width, height, ok := clampRect(frame, d.X, d.Y, d.Width, d.Height)
+	spans, full := buildSingleDamageSpans(frame, d)
-	if !ok {
+	if full {
-		return DeltaPlan{}
-	}
-	if height > maxPlannedDamageSpans {
 		return DeltaPlan{Snapshot: true}
 	}
-	spans := make([]Span, height)
-	for row := range height {
-		spans[row] = Span{Y: y + row, X: x, Width: width}
-	}
 	plan := DeltaPlan{Spans: spans}
-	if (frame.Height == 1 || len(spans) > 1) && deltaCostsSnapshot(frame, spans, false) {
+	if deltaCostsSnapshot(frame, spans, false) {
 		plan.Snapshot = true
 	}
 	return plan

M pkg/renderer/delta_plan_test.go +10 -0


@@ -85,6 +85,16 @@ func TestPlanDelta(t *testing.T) {
85	85		Spans: []Span{{Y: 6, X: 0, Width: 4}},
86	86		},
87	87		},
	88	+	{
	89	+	name: "safe full width scroll with expensive delta snapshots",
	90	+	frame: testFrame("1111", "new!"),
	91	+	damage: []Damage{
	92	+	{Kind: DamageScrollUp, X: 0, Y: 0, Width: 4, Height: 2, Count: 1},
	93	+	{Kind: DamageText, X: 0, Y: 0, Width: 4, Height: 2},
	94	+	},
	95	+	committed: testFrame("0000", "1111"),
	96	+	want: DeltaPlan{Snapshot: true},
	97	+	},
88	98		{
89	99		name: "rectangular scroll snapshots",
90	100		frame: scrolled,


@@ -85,6 +85,16 @@ func TestPlanDelta(t *testing.T) {
85	Spans: []Span{{Y: 6, X: 0, Width: 4}},	85		Spans: []Span{{Y: 6, X: 0, Width: 4}},
86	},	86		},
87	},	87		},
		88	+	{
		89	+	name: "safe full width scroll with expensive delta snapshots",
		90	+	frame: testFrame("1111", "new!"),
		91	+	damage: []Damage{
		92	+	{Kind: DamageScrollUp, X: 0, Y: 0, Width: 4, Height: 2, Count: 1},
		93	+	{Kind: DamageText, X: 0, Y: 0, Width: 4, Height: 2},
		94	+	},
		95	+	committed: testFrame("0000", "1111"),
		96	+	want: DeltaPlan{Snapshot: true},
		97	+	},
88	{	98		{
89	name: "rectangular scroll snapshots",	99		name: "rectangular scroll snapshots",
90	frame: scrolled,	100		frame: scrolled,

M pkg/renderer/renderer.go +7 -20


@@ -59,30 +59,14 @@ func (p *PreparedDraw) Commit() {
59	59		}
60	60
61	61		// Prepare plans and encodes a transactional draw. The renderer advances only
62		-	// when the returned draw is committed.
	62	+	// when the returned draw is committed. Keep at most one prepared draw
	63	+	// outstanding; commit or discard it before calling Prepare again.
63	64		func (r *Renderer) Prepare(frame Frame, damage []Damage, reset bool) (PreparedDraw, error) {
64	65		var candidate DeltaCandidate
65	66		var err error
66	67		if !reset && len(r.shadow) != 0 && r.width == frame.Width && r.height == frame.Height && len(damage) == 1 && (damage[0].Kind == DamageText \|\| damage[0].Kind == DamageClear) {
67	68		if err = frame.Validate(); err == nil {
68		-	d := damage[0]
69		-	x, y, width, height, ok := clampRect(frame, d.X, d.Y, d.Width, d.Height)
70		-	plan := DeltaPlan{}
71		-	if ok {
72		-	if height > maxPlannedDamageSpans {
73		-	plan.Snapshot = true
74		-	} else {
75		-	spans := make([]Span, height)
76		-	for row := range height {
77		-	spans[row] = Span{Y: y + row, X: x, Width: width}
78		-	}
79		-	if (frame.Height == 1 \|\| len(spans) > 1) && deltaCostsSnapshot(frame, spans, false) {
80		-	plan.Snapshot = true
81		-	}
82		-	plan.Spans = spans
83		-	}
84		-	}
85		-	candidate = DeltaCandidate{Plan: plan, frame: frame}
	69	+	candidate = DeltaCandidate{Plan: planSingleDamage(frame, damage[0]), frame: frame}
86	70		}
87	71		} else {
88	72		candidate, err = PlanDelta(frame, damage, r.committedFrame(), reset \|\| len(r.shadow) == 0)

@@ -96,7 +80,10 @@ func (r *Renderer) Prepare(frame Frame, damage []Damage, reset bool) (PreparedDr
96	80		return prepared, nil
97	81		}
98	82
99		-	buf := bufferPool.Get().(*bytes.Buffer)
	83	+	buf, ok := bufferPool.Get().(*bytes.Buffer)
	84	+	if !ok {
	85	+	buf = new(bytes.Buffer)
	86	+	}
100	87		buf.Reset()
101	88		defer putBuffer(buf)
102	89		if r.caps.SynchronizedOutput {

 }
 // Prepare plans and encodes a transactional draw. The renderer advances only
-// when the returned draw is committed.
+// when the returned draw is committed. Keep at most one prepared draw
+// outstanding; commit or discard it before calling Prepare again.
 func (r *Renderer) Prepare(frame Frame, damage []Damage, reset bool) (PreparedDraw, error) {
 	var candidate DeltaCandidate
 	var err error
 	if !reset && len(r.shadow) != 0 && r.width == frame.Width && r.height == frame.Height && len(damage) == 1 && (damage[0].Kind == DamageText || damage[0].Kind == DamageClear) {
 		if err = frame.Validate(); err == nil {
-			d := damage[0]
+			candidate = DeltaCandidate{Plan: planSingleDamage(frame, damage[0]), frame: frame}
-			x, y, width, height, ok := clampRect(frame, d.X, d.Y, d.Width, d.Height)
-			plan := DeltaPlan{}
-			if ok {
-				if height > maxPlannedDamageSpans {
-					plan.Snapshot = true
-				} else {
-					spans := make([]Span, height)
-					for row := range height {
-						spans[row] = Span{Y: y + row, X: x, Width: width}
-					}
-					if (frame.Height == 1 || len(spans) > 1) && deltaCostsSnapshot(frame, spans, false) {
-						plan.Snapshot = true
-					}
-					plan.Spans = spans
-				}
-			}
-			candidate = DeltaCandidate{Plan: plan, frame: frame}
 		}
 	} else {
 		candidate, err = PlanDelta(frame, damage, r.committedFrame(), reset || len(r.shadow) == 0)
 		return prepared, nil
 	}
-	buf := bufferPool.Get().(*bytes.Buffer)
+	buf, ok := bufferPool.Get().(*bytes.Buffer)
+	if !ok {
+		buf = new(bytes.Buffer)
+	}
 	buf.Reset()
 	defer putBuffer(buf)
 	if r.caps.SynchronizedOutput {

M pkg/renderer/scroll.go +1 -1


@@ -15,7 +15,7 @@ func findSafeScroll(frame Frame, damage []Damage) (Damage, bool) {
15	15		}
16	16
17	17		func isSafeScroll(frame Frame, d Damage) bool {
18		-	return d.X == 0 && d.Width == frame.Width && d.Y >= 0 && d.Height > 0 && d.Count > 0 && d.Count < d.Height && d.Y+d.Height <= frame.Height
	18	+	return d.X == 0 && d.Width == frame.Width && d.Y >= 0 && d.Y <= frame.Height && d.Height > 0 && d.Height <= frame.Height-d.Y && d.Count > 0 && d.Count < d.Height
19	19		}
20	20
21	21		func emitScrollUp(out *bytes.Buffer, d Damage) {


@@ -15,7 +15,7 @@ func findSafeScroll(frame Frame, damage []Damage) (Damage, bool) {
15		}	15		}
16			16
17		func isSafeScroll(frame Frame, d Damage) bool {	17		func isSafeScroll(frame Frame, d Damage) bool {
18	-	return d.X == 0 && d.Width == frame.Width && d.Y >= 0 && d.Height > 0 && d.Count > 0 && d.Count < d.Height && d.Y+d.Height <= frame.Height	18	+	return d.X == 0 && d.Width == frame.Width && d.Y >= 0 && d.Y <= frame.Height && d.Height > 0 && d.Height <= frame.Height-d.Y && d.Count > 0 && d.Count < d.Height
19		}	19		}
20			20
21		func emitScrollUp(out *bytes.Buffer, d Damage) {	21		func emitScrollUp(out *bytes.Buffer, d Damage) {