-
Notifications
You must be signed in to change notification settings - Fork 2.4k
mysql/replication: bound the intervals preallocation in ParseMysql56GTIDSet #20932
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
0307181
bcec182
9c3935d
def9237
afb24d4
e7f8f5e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,8 +17,10 @@ limitations under the License. | |
| package replication | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "maps" | ||
| "reflect" | ||
| "runtime" | ||
| "strings" | ||
| "testing" | ||
|
|
||
|
|
@@ -902,3 +904,74 @@ func TestSIDs(t *testing.T) { | |
| require.Len(t, sids, 1) | ||
| assert.Equal(t, "8bc65cca-3fe4-11ed-bbfb-091034d48b3e", sids[0].String()) | ||
| } | ||
|
|
||
| // TestParseMysql56GTIDSetIntervalsCapHint checks that the preallocation hint for | ||
| // the intervals slice does not follow a count taken from the input beyond what that | ||
| // input could hold, while a genuinely long interval list still parses unchanged. | ||
| // The hostile case asserts on allocation volume, because a colon run parses without | ||
| // error either way once the intervals are all discarded. | ||
| func TestParseMysql56GTIDSetIntervalsCapHint(t *testing.T) { | ||
| const sid = "00010203-0405-0607-0809-0a0b0c0d0e0f" | ||
|
|
||
| // Results must be identical either side of the bound. | ||
| for _, n := range []int{1, 10, 100, 1023, 1024, 1025, 2051, 5000} { | ||
| var sb strings.Builder | ||
| sb.WriteString(sid) | ||
| for i := range n { | ||
| // non-overlapping ascending intervals, so none are merged or discarded | ||
| fmt.Fprintf(&sb, ":%d-%d", 2*i+1, 2*i+1) | ||
| } | ||
| got, err := ParseMysql56GTIDSet(sb.String()) | ||
| require.NoError(t, err, "n=%d", n) | ||
| sidVal, err := ParseSID(sid) | ||
| require.NoError(t, err) | ||
| assert.Len(t, got[sidVal], n, "n=%d", n) | ||
|
Comment on lines
+924
to
+928
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This test passes unchanged against the parent implementation and therefore cannot guard the allocation fix: every generated AGENTS.md reference: AGENTS.md:L79-L80 Useful? React with 👍 / 👎. |
||
| } | ||
|
|
||
| // A long run of colons carries no intervals at all, so reserving one per colon | ||
| // is pure waste. 1MiB of colons is 16MiB of interval structs unbounded. | ||
| // | ||
| // The bound cannot be tightened past len(tail)/2 to shrink this further: the | ||
| // densest valid list is one singleton per two bytes ("1:"), so a smaller | ||
| // divisor under-reserves legitimate input. That trade-off is why the threshold | ||
| // here is 12MiB rather than the 8MiB a len(tail)/4 bound would reach -- | ||
| // measured, /4 cut this case to 5.0MiB but inflated a valid 100k-singleton | ||
| // list from 1,606,032 to 6,685,104 bytes. Bounding a hostile input is not | ||
| // worth a 4.16x regression on a valid one. | ||
| hostile := sid + ":" + strings.Repeat(":", 1<<20) | ||
| var before, after runtime.MemStats | ||
| runtime.GC() | ||
| runtime.ReadMemStats(&before) | ||
| _, _ = ParseMysql56GTIDSet(hostile) | ||
| runtime.ReadMemStats(&after) | ||
| allocated := after.TotalAlloc - before.TotalAlloc | ||
| assert.Less(t, allocated, uint64(12<<20), | ||
| "parsing %d colons allocated %d bytes for intervals that are all discarded", | ||
| 1<<20, allocated) | ||
|
|
||
| // The bound must not under-reserve a valid list. parseInterval accepts a | ||
| // singleton "N", so "1:1:1:..." needs one interval per two input bytes -- the | ||
| // densest valid form. Note that duplicate singletons are each retained rather | ||
| // than merged, so this really does need `singletons` intervals. | ||
| const singletons = 100000 | ||
| var sb strings.Builder | ||
| sb.WriteString(sid) | ||
| for range singletons { | ||
| sb.WriteString(":1") | ||
| } | ||
| runtime.GC() | ||
| runtime.ReadMemStats(&before) | ||
| dense, err := ParseMysql56GTIDSet(sb.String()) | ||
| runtime.ReadMemStats(&after) | ||
| require.NoError(t, err) | ||
| denseAlloc := after.TotalAlloc - before.TotalAlloc | ||
| sidVal, err := ParseSID(sid) | ||
| require.NoError(t, err) | ||
| assert.Len(t, dense[sidVal], singletons, | ||
| "duplicate singletons are retained, not merged") | ||
| // 100k intervals at 16 bytes is 1.6MiB; allow headroom for the strings but not | ||
| // for a doubling-and-copying append. | ||
| assert.Less(t, denseAlloc, uint64(3<<20), | ||
| "a dense valid singleton list allocated %d bytes, which means the capacity "+ | ||
| "hint under-reserved and append had to grow", denseAlloc) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless the originating task explicitly requested source-level commentary, this newly added multi-paragraph rationale violates the repository’s strict instruction not to add explanatory comments; the machine- and implementation-specific allocation measurements also risk becoming stale while duplicating PR/commit context. Keep the implementation focused and move this narrative out of the source.
AGENTS.md reference: AGENTS.md:L20-L27
Useful? React with 👍 / 👎.