mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-10 23:15:40 +00:00
perf(documents): stop counting every linker's occurrences twice (BUG-2798)
Codex round 13, on what the guard costs the SUCCESS path rather than what it blocks on the failure path. The cascade counts occurrences because the size guard needs that number before it is willing to build anything. It then handed the same content to links.ReplaceTitle, whose strings.Replace with n < 0 counts it again. Every ordinary rename therefore paid a second full pass over every linking document for a number it already had. links.ReplaceTitleN takes the count the caller already computed. It is a separate function rather than an optional parameter because the obligation is real and silent when broken: passing a number that is too small does not error, it leaves later occurrences unrewritten, which on this path means links left pointing at a title that no longer exists. A name at the call site is cheaper than a comment nobody reads. NO measured speedup is claimed, and the doc comment says so. This removes one linear pass from a path that also allocates a full copy of the same content and issues a write per linker, so the saving is real but not obviously significant. It is here because doing the same work twice needs a reason and there was not one — not because a benchmark asked for it. The test asserts equivalence with ReplaceTitle across several shapes, including the new-title-embeds-old case, and its counterfactual leg asserts that an under-count visibly DIVERGES — if it did not, the caller's obligation would be imaginary and the API misleading. Round 13 also confirmed two things worth recording: no quadratic scan across linkers, and the ESCAPE clause does not materially change the query plan because the leading `%` already forced a content scan. Gates: `go test ./...` under Postgres 17 EXIT=0; gofmt clean; `make lint` 0 issues. BUG-2798 Claude-Session: https://claude.ai/code/session_011T365kP1N9V88y15HxL4YN
This commit is contained in:
@@ -17,6 +17,30 @@ func ReplaceTitle(content, oldTitle, newTitle string) string {
|
||||
return replaceAll(content, old, new)
|
||||
}
|
||||
|
||||
// ReplaceTitleN is ReplaceTitle for a caller that has ALREADY counted the
|
||||
// occurrences, and it exists to stop that work being done twice.
|
||||
//
|
||||
// strings.Replace with n < 0 counts the string itself before building the
|
||||
// result. The document rename cascade counts first anyway — its size guard
|
||||
// needs the number before it is willing to build anything — so letting
|
||||
// Replace re-count adds a full pass over every linking document on the
|
||||
// success path (codex round 13).
|
||||
//
|
||||
// n MUST be the count of `[[oldTitle]]` in this exact content. Passing a
|
||||
// smaller number silently leaves later occurrences unrewritten, which is why
|
||||
// this is a separate function rather than an optional parameter on the one
|
||||
// above: the obligation is visible at the call site.
|
||||
//
|
||||
// No measured speedup is claimed. This removes one linear pass from a path
|
||||
// that also allocates a full copy of the same content and issues a write per
|
||||
// linker, so the saving is real but not obviously significant; it is here
|
||||
// because doing the same work twice needs a reason and there was not one.
|
||||
func ReplaceTitleN(content, oldTitle, newTitle string, n int) string {
|
||||
old := "[[" + oldTitle + "]]"
|
||||
new := "[[" + newTitle + "]]"
|
||||
return strings.Replace(content, old, new, n)
|
||||
}
|
||||
|
||||
// RewriteWikiTitle rewrites the four title-form wiki-link shapes that
|
||||
// resolve to an item titled `oldTitle` in collection `collSlug`,
|
||||
// substituting `newTitle` for the title portion and preserving any
|
||||
|
||||
@@ -71,3 +71,38 @@ func TestReplaceTitle_StillRewritesEveryOccurrence(t *testing.T) {
|
||||
t.Errorf("an occurrence survived: %q", got)
|
||||
}
|
||||
}
|
||||
|
||||
// TestReplaceTitleN_MatchesReplaceTitleWhenGivenTheTrueCount pins the
|
||||
// obligation ReplaceTitleN puts on its caller: given the real occurrence
|
||||
// count, it must produce exactly what ReplaceTitle produces.
|
||||
//
|
||||
// The under-count leg is the counterfactual, and it is why the two are
|
||||
// separate functions rather than one with an optional parameter — passing a
|
||||
// number that is too small does not error, it silently leaves later
|
||||
// occurrences unrewritten, which on the rename path means links left pointing
|
||||
// at a title that no longer exists.
|
||||
func TestReplaceTitleN_MatchesReplaceTitleWhenGivenTheTrueCount(t *testing.T) {
|
||||
for _, tc := range []struct{ name, content, old, new string }{
|
||||
{"several occurrences", "a [[Old]] b [[Old]] c [[Old]] d", "Old", "New"},
|
||||
{"none", "nothing to see here", "Old", "New"},
|
||||
{"new embeds old", "x [[A]] y", "A", "A]] [[A"},
|
||||
{"shrinking", "[[LongOldTitle]] and [[LongOldTitle]]", "LongOldTitle", "n"},
|
||||
} {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
want := ReplaceTitle(tc.content, tc.old, tc.new)
|
||||
n := strings.Count(tc.content, "[["+tc.old+"]]")
|
||||
if got := ReplaceTitleN(tc.content, tc.old, tc.new, n); got != want {
|
||||
t.Errorf("ReplaceTitleN with the true count %d:\n got: %q\nwant: %q", n, got, want)
|
||||
}
|
||||
|
||||
// Under-counting must visibly diverge, or the count is not
|
||||
// load-bearing and this function has no contract worth stating.
|
||||
if n > 1 {
|
||||
if got := ReplaceTitleN(tc.content, tc.old, tc.new, n-1); got == want {
|
||||
t.Errorf("ReplaceTitleN with a count one too low produced the correct result; " +
|
||||
"the caller's obligation is not real, so the API is misleading")
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -687,7 +687,7 @@ func (s *Store) updateLinksInTx(tx *sql.Tx, workspaceID, oldTitle, newTitle stri
|
||||
return newRenameCascadeTooLargeError(newTitle, retained)
|
||||
}
|
||||
|
||||
du.rewritten = links.ReplaceTitle(du.read, oldTitle, newTitle)
|
||||
du.rewritten = links.ReplaceTitleN(du.read, oldTitle, newTitle, int(occurrences))
|
||||
updates = append(updates, du)
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
|
||||
Reference in New Issue
Block a user