Skip to content

Commit f357911

Browse files
author
gluck
committed
store: ReadPage on a raw log reads the page, not the log
xwalLog.ReadPage was readPage(l.Read(), ...): it decoded every record in the channel to return n of them. Every caller asking for "the last record" was walking the whole conversation, while the comments beside them said it cost one read: the dormant reader's tail probe (on every read of a dormant aria), the agent's, the IR door's pages, the composed cache's seed and bracket reads of an ancestor. The aria's own log is a tree log in front of the same channel and delegates ReadPage to it, so it paid the same. The channel is addressed by its own LT, so the window is arithmetic on the bounds: backward from before-1 until n records are found, forward from max(first, from). A record that does not read is skipped as Read skipped it, so the page reaches past it exactly as the slice did. total is the channel's extent, as Len reports it and as the tree log already returned. TestReadPageByPositionMatchesTheSlice asks every (from, before, n) shape of every aria in random fork chains, raw and through the aria's log, against readPage over Read(); two off-by-one breakages were canaried and caught. BenchmarkReadPage: a tail read is ~18-20 us at 300 records and at 3000. TurnAtLT and the lineage probe keep their positional reads; their comments no longer cite this bug.
1 parent 0fc7005 commit f357911

6 files changed

Lines changed: 201 additions & 13 deletions

File tree

‎internal/angelus/lineage_bench_test.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,12 @@ import (
2222
// long conversation, then a CHAIN of forks, each forked again after its own
2323
// turn, the leaf read against its parent. Two shapes:
2424
//
25-
// head each link a head fork of an abandoned parent: the base sits past
26-
// the end of the parent's log, which used to mean a walk of that
27-
// log from LT 1 for every link, unmemoized, on every hop.
28-
// interior each link cut at a turn the parent has: the base is a real
29-
// record, which the old memo did remember, so this shape isolates
30-
// the other cost: listing every trunk in the store per read.
25+
// head each link a head fork of an abandoned parent: the base sits past
26+
// the end of the parent's log, which used to mean a walk of that
27+
// log from LT 1 for every link, unmemoized, on every hop.
28+
// interior each link cut at a turn the parent has: the base is a real
29+
// record, which the old memo did remember, so this shape isolates
30+
// the other cost: listing every trunk in the store per read.
3131
//
3232
// unrelated is how many other arias share the store, which is what the
3333
// listing paid for.

‎internal/angelus/lineage_handlers.go‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,9 +53,7 @@ func (a *Angelus) baseTurnOf(parent string, l store.Link) uint64 {
5353
}
5454
if l.BaseTurnKnown {
5555
// Recorded past the end. The parent's tail says whether it has
56-
// written at the base since; until it has, the record stands. The
57-
// tail is a positional read, not ReadPage, which on a raw node log
58-
// materializes the whole log to return one record.
56+
// written at the base since; until it has, the record stands.
5957
if tail, ok := log.PeekTail(); !ok || tail.LT < l.BaseLT {
6058
return l.BaseTurn
6159
}

‎internal/store/forkturn_test.go‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -369,3 +369,71 @@ func onlySidecar(t *testing.T, root string) string {
369369
}
370370
return found[0]
371371
}
372+
373+
// READPAGE BY POSITION IS THE SAME PAGE AS THE WHOLE-LOG SLICE. The raw log's
374+
// ReadPage used to decode every record and slice; it reads only the window
375+
// now. Every (from, before, n) shape is asked of every aria in random fork
376+
// chains (whose prefixes are read through their parents), raw and through the
377+
// aria's own log, and compared with readPage over Read().
378+
func TestReadPageByPositionMatchesTheSlice(t *testing.T) {
379+
for seed := int64(1); seed <= 12; seed++ {
380+
rng := rand.New(rand.NewSource(seed))
381+
be, cur, log := newConversation(t)
382+
chain := []string{cur}
383+
turn := uint64(0)
384+
for hop := 0; hop < 3; hop++ {
385+
for i := 0; i < 2+rng.Intn(5); i++ {
386+
turn++
387+
put(t, log, ask(turn, "q"))
388+
put(t, log, answer(turn, "a"))
389+
}
390+
at := uint64(0)
391+
if rng.Intn(2) == 0 {
392+
tail, _ := log.ReadPage(0, 1<<62, 1)
393+
at = 1 + uint64(rng.Int63n(int64(tail[0].LT)))
394+
}
395+
child, _, err := be.ForkWith(cur, at, birth("c"))
396+
if err != nil {
397+
t.Fatal(err)
398+
}
399+
cur = child
400+
chain = append(chain, cur)
401+
log, _ = be.OpenFigIR(cur)
402+
}
403+
for _, id := range chain {
404+
aria, _ := be.OpenFigIR(id)
405+
for name, l := range map[string]Log[message.Message]{"raw": be.OpenNode(id), "aria": aria} {
406+
all := l.Read()
407+
if len(all) == 0 {
408+
continue
409+
}
410+
top := all[len(all)-1].LT
411+
for q := 0; q < 200; q++ {
412+
from, before, n := uint64(0), uint64(0), rng.Intn(6)
413+
switch rng.Intn(4) {
414+
case 0:
415+
before = 1 + uint64(rng.Int63n(int64(top+3)))
416+
case 1:
417+
before = ^uint64(0)
418+
case 2:
419+
from = uint64(rng.Int63n(int64(top + 3)))
420+
case 3:
421+
from = 0
422+
}
423+
got, _ := l.ReadPage(from, before, n)
424+
want, _ := readPage(all, from, before, n)
425+
if len(got) != len(want) {
426+
t.Fatalf("seed %d %s %s ReadPage(%d, %d, %d): %d entries, the slice has %d",
427+
seed, name, id, from, before, n, len(got), len(want))
428+
}
429+
for i := range got {
430+
if got[i].LT != want[i].LT || got[i].Payload.TurnID != want[i].Payload.TurnID {
431+
t.Fatalf("seed %d %s %s ReadPage(%d, %d, %d)[%d]: LT %d turn %d, the slice has LT %d turn %d",
432+
seed, name, id, from, before, n, i, got[i].LT, got[i].Payload.TurnID, want[i].LT, want[i].Payload.TurnID)
433+
}
434+
}
435+
}
436+
}
437+
}
438+
}
439+
}
Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
package store
2+
3+
import (
4+
"fmt"
5+
"strings"
6+
"testing"
7+
8+
"github.com/jack-work/figaro/api/message"
9+
)
10+
11+
// READPAGE, by the shapes its callers actually ask. The tail probe (0, ^0, 1)
12+
// is what the dormant reader runs on every read of an aria and the agent runs
13+
// on its own log; a tail page (0, ^0, 32) is a backward page of history; a
14+
// forward page from the middle is the composed cache filling an LT bracket of
15+
// an ancestor. raw is the node log the composed cache opens for an ancestor;
16+
// aria is the aria's own log, the tree log in front of the same channel.
17+
//
18+
// Compiles against the tree before the positional ReadPage, so benchstat
19+
// compares like with like.
20+
func BenchmarkReadPage(b *testing.B) {
21+
prose := strings.Repeat("a sentence of ordinary length in a reply. ", 24) // ~1 KB
22+
for _, history := range []int{300, 3000} {
23+
be, id, log := newConversation(b)
24+
for t := uint64(1); t <= uint64(history/3); t++ {
25+
put(b, log, ask(t, "question"))
26+
put(b, log, answer(t, prose))
27+
put(b, log, answer(t, prose))
28+
}
29+
tail, _ := log.ReadPage(0, ^uint64(0), 1)
30+
mid := tail[0].LT / 2
31+
aria, _ := be.OpenFigIR(id)
32+
for _, src := range []struct {
33+
name string
34+
log Log[message.Message]
35+
}{{"raw", be.OpenNode(id)}, {"aria", aria}} {
36+
for _, q := range []struct {
37+
name string
38+
from, before uint64
39+
n int
40+
}{
41+
{"tail-1", 0, ^uint64(0), 1},
42+
{"tail-32", 0, ^uint64(0), 32},
43+
{"forward-32", mid, 0, 32},
44+
} {
45+
b.Run(fmt.Sprintf("log=%s/query=%s/history=%d", src.name, q.name, history), func(b *testing.B) {
46+
b.ReportAllocs()
47+
for b.Loop() {
48+
if page, _ := src.log.ReadPage(q.from, q.before, q.n); len(page) == 0 {
49+
b.Fatal("empty page")
50+
}
51+
}
52+
})
53+
}
54+
}
55+
}
56+
}

‎internal/store/turnat.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,8 @@ import (
2222
// It used to be a walk from LT 1 whenever the base was past the end of the
2323
// parent's log, which is every head fork, on every hop between two arias.
2424
//
25-
// ONLY INDEXED READS. PeekTail and ReadFrom address the channel by position;
26-
// ReadPage, on a raw node log, materializes the whole log and slices it, so
27-
// the backward read here is built from ReadFrom windows and never from it.
25+
// ONLY POSITIONAL READS: PeekTail and ReadFrom windows, each of which reads
26+
// the records it returns and nothing else.
2827
//
2928
// Were the invariant ever broken (an unstamped question below lt), the answer
3029
// would be the stamp BELOW that question: a smaller turn than the truth. For

‎internal/store/xwal_log.go‎

Lines changed: 68 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -286,8 +286,75 @@ func (l *xwalLog[T]) ReadFrom(lt uint64, n int) []Entry[T] {
286286
return out
287287
}
288288

289+
// ReadPage is readPage's contract read by POSITION: only the records the page
290+
// returns are read and decoded.
291+
//
292+
// It used to be readPage(l.Read(), ...), which decoded every record in the
293+
// channel to return n of them. Every caller that asked for "the last record"
294+
// (the dormant reader's tail probe, the agent's, the composed cache's seed for
295+
// an ancestor range) was walking the whole conversation, while the comments
296+
// beside them said it cost one read. The channel is addressed by its own LT
297+
// (see ReadFrom), so the window is arithmetic on the bounds.
298+
//
299+
// total is the channel's extent, as Len reports it: the number the tree log
300+
// in front of this already returns. It counted DECODABLE records before, which
301+
// is the same number unless a record fails to decode, and counting those meant
302+
// decoding all of them.
289303
func (l *xwalLog[T]) ReadPage(from, before uint64, n int) ([]Entry[T], int) {
290-
return readPage(l.Read(), from, before, n)
304+
var out []Entry[T]
305+
total := 0
306+
_ = l.openOnce(func(xw *xwal.XWAL) error {
307+
first, last, ok := channelBounds(xw, l.channel)
308+
if !ok {
309+
return nil
310+
}
311+
total = int(last-first) + 1
312+
read := func(lt uint64) (Entry[T], bool) {
313+
r, err := xw.ReadAt(l.channel, lt)
314+
if err != nil {
315+
return Entry[T]{}, false
316+
}
317+
return decodeRecord[T](r)
318+
}
319+
if before > 0 {
320+
if n <= 0 {
321+
return nil
322+
}
323+
// Backward from before-1, until n are found or the channel ends.
324+
// A record that does not read is skipped, as Read skips it, so the
325+
// page reaches past it exactly as the whole-log slice did.
326+
top := last
327+
if before-1 < top {
328+
top = before - 1
329+
}
330+
for lt := top; lt >= first && len(out) < n; lt-- {
331+
if e, ok := read(lt); ok {
332+
out = append(out, e)
333+
}
334+
if lt == 0 {
335+
break
336+
}
337+
}
338+
for i, j := 0, len(out)-1; i < j; i, j = i+1, j-1 {
339+
out[i], out[j] = out[j], out[i]
340+
}
341+
return nil
342+
}
343+
start := first
344+
if from > start {
345+
start = from
346+
}
347+
for lt := start; lt <= last && (n <= 0 || len(out) < n); lt++ {
348+
if e, ok := read(lt); ok {
349+
out = append(out, e)
350+
}
351+
}
352+
return nil
353+
})
354+
if out == nil && before > 0 && n > 0 {
355+
out = []Entry[T]{}
356+
}
357+
return out, total
291358
}
292359

293360
// TailSnapshot is the last n entries, ascending, read from the tail

0 commit comments

Comments
 (0)