cc11117 fix(core): release CopyFrom storage when the source shrinks · vev-vt

M core/frame_copy.go +15 -16


@@ -5,12 +5,14 @@ import "maps"
5	5		// CopyFrom makes f an independent structural copy of src, like Replace, but
6	6		// reuses f's existing page storage (cells, row descriptors, style and payload
7	7		// tables and dictionaries) when f already owns a page. It therefore performs no
8		-	// allocation once f's page has grown to src's size.
	8	+	// allocation once f's page has grown to src's size. A page more than four times
	9	+	// larger than src is replaced by a clone, so shrinking a terminal releases the
	10	+	// old storage.
9	11		//
10	12		// Page-local style and payload IDs and physical row rotation are copied
11		-	// verbatim, exactly as Clone does. Every Frame value sharing f's page observes
12		-	// the overwrite. An invalid or empty source leaves f unchanged. Copying a frame
13		-	// onto itself only normalizes the dimensions.
	13	+	// verbatim, exactly as Clone does. When the page is reused, every Frame value
	14	+	// sharing f's page observes the overwrite. An invalid or empty source leaves f
	15	+	// unchanged. Copying a frame onto itself only normalizes the dimensions.
14	16		func (f *Frame) CopyFrom(src Frame) {
15	17		if f == nil {
16	18		return

@@ -18,15 +20,16 @@ func (f *Frame) CopyFrom(src Frame) {
18	20		if err := src.validateStorage(); err != nil \|\| src.Width <= 0 \|\| src.Height <= 0 {
19	21		return
20	22		}
21		-	if f.page == nil {
	23	+	if f.page == src.page {
	24	+	f.Width, f.Height = src.Width, src.Height
	25	+	return
	26	+	}
	27	+	if f.page == nil \|\| cap(f.page.cells) > 4*len(src.page.cells) {
22	28		*f = src.Clone()
23	29		return
24	30		}
25	31		f.Width, f.Height = src.Width, src.Height
26	32		dst, from := f.page, src.page
27		-	if dst == from {
28		-	return
29		-	}
30	33		dst.cells = append(dst.cells[:0], from.cells...)
31	34		dst.rows = append(dst.rows[:0], from.rows...)
32	35		dst.styles = append(dst.styles[:0], from.styles...)

@@ -45,16 +48,12 @@ func (f *Frame) CopyFrom(src Frame) {
45	48		dst.payloads = append(dst.payloads[:0], from.payloads...)
46	49		dst.freePayloads = append(dst.freePayloads[:0], from.freePayloads...)
47	50		dst.payloadBytes = from.payloadBytes
48		-	if len(from.payloadIndex) == 0 {
49		-	clear(dst.payloadIndex)
	51	+	if dst.payloadIndex == nil && len(from.payloadIndex) > 0 {
	52	+	dst.payloadIndex = make(map[CellPayload]uint32, len(from.payloadIndex))
50	53		} else {
51		-	if dst.payloadIndex == nil {
52		-	dst.payloadIndex = make(map[CellPayload]uint32, len(from.payloadIndex))
53		-	} else {
54		-	clear(dst.payloadIndex)
55		-	}
56		-	maps.Copy(dst.payloadIndex, from.payloadIndex)
	54	+	clear(dst.payloadIndex)
57	55		}
	56	+	maps.Copy(dst.payloadIndex, from.payloadIndex)
58	57		}
59	58
60	59		// RowsEqualAt reports whether logical row ya of a and logical row yb of b hold

 // CopyFrom makes f an independent structural copy of src, like Replace, but
 // reuses f's existing page storage (cells, row descriptors, style and payload
 // tables and dictionaries) when f already owns a page. It therefore performs no
-// allocation once f's page has grown to src's size.
+// allocation once f's page has grown to src's size. A page more than four times
+// larger than src is replaced by a clone, so shrinking a terminal releases the
+// old storage.
 //
 // Page-local style and payload IDs and physical row rotation are copied
-// verbatim, exactly as Clone does. Every Frame value sharing f's page observes
+// verbatim, exactly as Clone does. When the page is reused, every Frame value
-// the overwrite. An invalid or empty source leaves f unchanged. Copying a frame
+// sharing f's page observes the overwrite. An invalid or empty source leaves f
-// onto itself only normalizes the dimensions.
+// unchanged. Copying a frame onto itself only normalizes the dimensions.
 func (f *Frame) CopyFrom(src Frame) {
 	if f == nil {
 		return
 	if err := src.validateStorage(); err != nil || src.Width <= 0 || src.Height <= 0 {
 		return
 	}
-	if f.page == nil {
+	if f.page == src.page {
+		f.Width, f.Height = src.Width, src.Height
+		return
+	}
+	if f.page == nil || cap(f.page.cells) > 4*len(src.page.cells) {
 		*f = src.Clone()
 		return
 	}
 	f.Width, f.Height = src.Width, src.Height
 	dst, from := f.page, src.page
-	if dst == from {
-		return
-	}
 	dst.cells = append(dst.cells[:0], from.cells...)
 	dst.rows = append(dst.rows[:0], from.rows...)
 	dst.styles = append(dst.styles[:0], from.styles...)
 	dst.payloads = append(dst.payloads[:0], from.payloads...)
 	dst.freePayloads = append(dst.freePayloads[:0], from.freePayloads...)
 	dst.payloadBytes = from.payloadBytes
-	if len(from.payloadIndex) == 0 {
+	if dst.payloadIndex == nil && len(from.payloadIndex) > 0 {
-		clear(dst.payloadIndex)
+		dst.payloadIndex = make(map[CellPayload]uint32, len(from.payloadIndex))
 	} else {
-		if dst.payloadIndex == nil {
+		clear(dst.payloadIndex)
-			dst.payloadIndex = make(map[CellPayload]uint32, len(from.payloadIndex))
-		} else {
-			clear(dst.payloadIndex)
-		}
-		maps.Copy(dst.payloadIndex, from.payloadIndex)
 	}
+	maps.Copy(dst.payloadIndex, from.payloadIndex)
 }
 // RowsEqualAt reports whether logical row ya of a and logical row yb of b hold

M core/frame_copy_test.go +25 -17


@@ -122,7 +122,21 @@ func TestCopyFromDoesNotAllocateOnceSized(t *testing.T) {
122	122		}
123	123		}
124	124
125		-	func TestRowsEqual(t *testing.T) {
	125	+	func TestCopyFromReleasesStorageWhenSourceShrinks(t *testing.T) {
	126	+	dst := copyTestFrame(t, 240, 80, 'a')
	127	+	small := copyTestFrame(t, 20, 5, 'b')
	128	+	dst.CopyFrom(small)
	129	+	if got := cap(dst.page.cells); got > 4*len(small.page.cells) {
	130	+	t.Fatalf("CopyFrom kept %d cells for a %d-cell source", got, len(small.page.cells))
	131	+	}
	132	+	for y := range small.Height {
	133	+	if !RowsEqualAt(dst, y, small, y) {
	134	+	t.Fatalf("row %d differs after shrinking copy", y)
	135	+	}
	136	+	}
	137	+	}
	138	+
	139	+	func TestRowsEqualAt(t *testing.T) {
126	140		a := copyTestFrame(t, 10, 4, 'a')
127	141		b := a.Clone()
128	142		for y := range a.Height {

@@ -195,13 +209,22 @@ func TestRowsEqual(t *testing.T) {
195	209		t.Fatal("equivalent styles reported different")
196	210		}
197	211
	212	+	// A row compares equal at a different index after scrolling.
	213	+	shifted := a.Clone()
	214	+	shifted.ScrollUp(0, a.Height-1, 1)
	215	+	for y := range a.Height - 1 {
	216	+	if !RowsEqualAt(a, y+1, shifted, y) {
	217	+	t.Fatalf("row %d of a != row %d of scrolled copy", y+1, y)
	218	+	}
	219	+	}
	220	+
198	221		// Mismatched shape or range is unequal and never panics.
199	222		if RowsEqualAt(a, 0, NewFrame(a.Width+1, a.Height), 0) \|\| RowsEqualAt(a, -1, b, -1) \|\| RowsEqualAt(a, a.Height, b, a.Height) \|\| RowsEqualAt(Frame{}, 0, Frame{}, 0) {
200	223		t.Fatal("invalid comparison reported equal")
201	224		}
202	225		}
203	226
204		-	func TestRowsEqualDoesNotAllocate(t *testing.T) {
	227	+	func TestRowsEqualAtDoesNotAllocate(t *testing.T) {
205	228		a := copyTestFrame(t, 80, 4, 'a')
206	229		b := a.Clone()
207	230		if n := testing.AllocsPerRun(20, func() { RowsEqualAt(a, 2, b, 2) }); n != 0 {

@@ -283,21 +306,6 @@ func TestCopyFromDropsStaleStyleCache(t *testing.T) {
283	306		}
284	307		}
285	308
286		-	func TestRowsEqualAt(t *testing.T) {
287		-	a := copyTestFrame(t, 10, 5, 'a')
288		-	b := a.Clone()
289		-	// b row y+1 shifted: compare a row y+1 against b row y after scrolling b.
290		-	b.ScrollUp(0, 4, 1)
291		-	for y := range 4 {
292		-	if !RowsEqualAt(a, y+1, b, y) {
293		-	t.Fatalf("row %d of a != row %d of scrolled b", y+1, y)
294		-	}
295		-	}
296		-	if RowsEqualAt(a, 0, b, 0) \|\| RowsEqualAt(a, 0, b, 5) \|\| RowsEqualAt(a, -1, b, 0) {
297		-	t.Fatal("unequal or out-of-range rows reported equal")
298		-	}
299		-	}
300		-
301	309		func TestRowsEqualAtRejectsCorruptPayloadIDs(t *testing.T) {
302	310		a := copyTestFrame(t, 4, 1, 'a')
303	311		b := a.Clone()

 	}
 }
-func TestRowsEqual(t *testing.T) {
+func TestCopyFromReleasesStorageWhenSourceShrinks(t *testing.T) {
+	dst := copyTestFrame(t, 240, 80, 'a')
+	small := copyTestFrame(t, 20, 5, 'b')
+	dst.CopyFrom(small)
+	if got := cap(dst.page.cells); got > 4*len(small.page.cells) {
+		t.Fatalf("CopyFrom kept %d cells for a %d-cell source", got, len(small.page.cells))
+	}
+	for y := range small.Height {
+		if !RowsEqualAt(dst, y, small, y) {
+			t.Fatalf("row %d differs after shrinking copy", y)
+		}
+	}
+}
+func TestRowsEqualAt(t *testing.T) {
 	a := copyTestFrame(t, 10, 4, 'a')
 	b := a.Clone()
 	for y := range a.Height {
 		t.Fatal("equivalent styles reported different")
 	}
+	// A row compares equal at a different index after scrolling.
+	shifted := a.Clone()
+	shifted.ScrollUp(0, a.Height-1, 1)
+	for y := range a.Height - 1 {
+		if !RowsEqualAt(a, y+1, shifted, y) {
+			t.Fatalf("row %d of a != row %d of scrolled copy", y+1, y)
+		}
+	}
 	// Mismatched shape or range is unequal and never panics.
 	if RowsEqualAt(a, 0, NewFrame(a.Width+1, a.Height), 0) || RowsEqualAt(a, -1, b, -1) || RowsEqualAt(a, a.Height, b, a.Height) || RowsEqualAt(Frame{}, 0, Frame{}, 0) {
 		t.Fatal("invalid comparison reported equal")
 	}
 }
-func TestRowsEqualDoesNotAllocate(t *testing.T) {
+func TestRowsEqualAtDoesNotAllocate(t *testing.T) {
 	a := copyTestFrame(t, 80, 4, 'a')
 	b := a.Clone()
 	if n := testing.AllocsPerRun(20, func() { RowsEqualAt(a, 2, b, 2) }); n != 0 {
 	}
 }
-func TestRowsEqualAt(t *testing.T) {
-	a := copyTestFrame(t, 10, 5, 'a')
-	b := a.Clone()
-	// b row y+1 shifted: compare a row y+1 against b row y after scrolling b.
-	b.ScrollUp(0, 4, 1)
-	for y := range 4 {
-		if !RowsEqualAt(a, y+1, b, y) {
-			t.Fatalf("row %d of a != row %d of scrolled b", y+1, y)
-		}
-	}
-	if RowsEqualAt(a, 0, b, 0) || RowsEqualAt(a, 0, b, 5) || RowsEqualAt(a, -1, b, 0) {
-		t.Fatal("unequal or out-of-range rows reported equal")
-	}
-}
 func TestRowsEqualAtRejectsCorruptPayloadIDs(t *testing.T) {
 	a := copyTestFrame(t, 4, 1, 'a')
 	b := a.Clone()

M docs/ownership.md +3 -1


@@ -19,7 +19,9 @@ should use these methods rather than assuming a particular memory layout.
19	19		Rows returned by public extraction methods are copies. For a writable
20	20		`core.Frame`, use `Set`, `WriteRow`, `FillRow`, `CopyRow` and the scroll operations.
21	21		A plain Go assignment of a `Frame` shares its storage; use `Clone()` when you need
22		-	an independent grid.
	22	+	an independent grid. `CopyFrom` reuses the destination's storage, so every
	23	+	`Frame` assigned from it sees the overwrite: call it only on a frame you own
	24	+	exclusively. `core.RowsEqualAt` compares rows of two frames without copying.
23	25
24	26		## Callbacks
25	27


@@ -19,7 +19,9 @@ should use these methods rather than assuming a particular memory layout.
19		Rows returned by public extraction methods are copies. For a writable	19		Rows returned by public extraction methods are copies. For a writable
20		`core.Frame`, use `Set`, `WriteRow`, `FillRow`, `CopyRow` and the scroll operations.	20		`core.Frame`, use `Set`, `WriteRow`, `FillRow`, `CopyRow` and the scroll operations.
21		A plain Go assignment of a `Frame` shares its storage; use `Clone()` when you need	21		A plain Go assignment of a `Frame` shares its storage; use `Clone()` when you need
22	-	an independent grid.	22	+	an independent grid. `CopyFrom` reuses the destination's storage, so every
			23	+	`Frame` assigned from it sees the overwrite: call it only on a frame you own
			24	+	exclusively. `core.RowsEqualAt` compares rows of two frames without copying.
23			25
24		## Callbacks	26		## Callbacks
25			27

M screen_evict_scratch_test.go +2 -1


@@ -7,7 +7,8 @@ import (
7	7		)
8	8
9	9		// OnLineEvicted callers may retain the slice: successive evictions must not
10		-	// share the Screen's internal scratch row.
	10	+	// share the Screen's internal scratch row. The oracle checks the same rule
	11	+	// across scenarios but is skipped under -short; this is the -short guard.
11	12		func TestOnLineEvictedRowsAreIndependentCopies(t *testing.T) {
12	13		s := NewScreen(4, 2)
13	14		var rows [][]renderer.Cell


@@ -7,7 +7,8 @@ import (
7		)	7		)
8			8
9		// OnLineEvicted callers may retain the slice: successive evictions must not	9		// OnLineEvicted callers may retain the slice: successive evictions must not
10	-	// share the Screen's internal scratch row.	10	+	// share the Screen's internal scratch row. The oracle checks the same rule
			11	+	// across scenarios but is skipped under -short; this is the -short guard.
11		func TestOnLineEvictedRowsAreIndependentCopies(t *testing.T) {	12		func TestOnLineEvictedRowsAreIndependentCopies(t *testing.T) {
12		s := NewScreen(4, 2)	13		s := NewScreen(4, 2)
13		var rows [][]renderer.Cell	14		var rows [][]renderer.Cell

M screen_graphics_test.go +2 -2


@@ -407,8 +407,6 @@ func TestScreenWriteReentrantFromCallbackKeepsOuterStreamIntact(t *testing.T) {
407	407		})
408	408		}
409	409
410		-	// The Screen borrows the caller's Write buffer while parsing; nothing it
411		-	// retains may alias that buffer once Write returns.
412	410		// Base64 payloads padded with CR/LF decode into a buffer sized for the raw
413	411		// input. The scene must not retain that slack beyond its accounted bytes,
414	412		// otherwise untrusted output could hold memory past MaxEncodedBytes.

@@ -434,6 +432,8 @@ func TestScreenKittyNewlinePaddedPayloadRetainsOnlyAccountedBytes(t *testing.T)
434	432		runtime.KeepAlive(screen)
435	433		}
436	434
	435	+	// The Screen borrows the caller's Write buffer while parsing; nothing it
	436	+	// retains may alias that buffer once Write returns.
437	437		func TestScreenKittyGraphicsDoesNotAliasCallerWriteBuffer(t *testing.T) {
438	438		scribble := func(b []byte) {
439	439		for i := range b {

 	})
 }
-// The Screen borrows the caller's Write buffer while parsing; nothing it
-// retains may alias that buffer once Write returns.
 // Base64 payloads padded with CR/LF decode into a buffer sized for the raw
 // input. The scene must not retain that slack beyond its accounted bytes,
 // otherwise untrusted output could hold memory past MaxEncodedBytes.
 	runtime.KeepAlive(screen)
 }
+// The Screen borrows the caller's Write buffer while parsing; nothing it
+// retains may alias that buffer once Write returns.
 func TestScreenKittyGraphicsDoesNotAliasCallerWriteBuffer(t *testing.T) {
 	scribble := func(b []byte) {
 		for i := range b {