diff --git a/app.go b/app.go index 12ee4e8..4e7c9d1 100644 --- a/app.go +++ b/app.go @@ -766,6 +766,8 @@ type App struct { liveTableMu sync.Mutex // guards liveTableFor liveTableFor *sql.DB // logbook whose live_status DDL has been ensured (once per connection, not per call) awardSnapMu sync.Mutex // guards the award QSO snapshot + awardSnapBuild sync.Mutex // serialises BUILDING it — see awardSnapshot + awardSnapCap int // rows the last build produced, the capacity hint for the next awardSnap []qso.QSO // light-scanned + enriched logbook snapshot reused across award computations awardSnapRev string // logbook revision the snapshot was built at ("" = none) awardSnapUsed time.Time // last read — the snapshot is dropped once it goes cold (see awardSnapshotJanitor) @@ -4348,8 +4350,41 @@ func (a *App) awardSnapshot() ([]qso.QSO, error) { a.awardSnapMu.Unlock() } + // ONE BUILDER AT A TIME. + // + // The cache lock above is released before the pull, so every caller that + // arrives while the logbook is being read used to miss and start its own. + // Opening the Awards panel does that: a field log showed three pulls of the + // same 123 615 QSOs within ten seconds, and three copies alive at once took + // the heap from 725 MB to 2.5 GB. The work was identical each time. + // + // Waiting here costs the second and third caller the seconds the first was + // going to take anyway — they were already paying that, plus a second and + // third trip to the database. + a.awardSnapBuild.Lock() + defer a.awardSnapBuild.Unlock() + // Re-check: whoever held the build lock has just finished, and their result + // is what we came for. + if revErr == nil { + a.awardSnapMu.Lock() + if a.awardSnap != nil && a.awardSnapRev == rev { + qs := a.awardSnap + a.awardSnapUsed = time.Now() + a.awardSnapMu.Unlock() + return qs, nil + } + a.awardSnapMu.Unlock() + } + t0 := time.Now() - var all []qso.QSO + // Sized from the last build. Growing a slice to 123 000 structs by doubling + // copies the whole thing a dozen times and holds the old and the new array + // together at every step — on the biggest object OpsLog keeps, that transient + // is worth avoiding. + a.awardSnapMu.Lock() + hint := a.awardSnapCap + a.awardSnapMu.Unlock() + all := make([]qso.QSO, 0, hint) if err := a.qso.IterateForAwards(a.ctx, func(q qso.QSO) error { a.enrichQSOForAwards(&q) all = append(all, q) @@ -4365,13 +4400,14 @@ func (a *App) awardSnapshot() ([]qso.QSO, error) { applog.Printf("awardSnapshot: pulled %d qsos from logbook in %v (rev=%s) — go heap now %d MB", len(all), time.Since(t0).Round(time.Millisecond), rev, ms.HeapAlloc/(1024*1024)) + a.awardSnapMu.Lock() + a.awardSnapCap = len(all) // next build starts the right size if revErr == nil { - a.awardSnapMu.Lock() a.awardSnap = all a.awardSnapRev = rev a.awardSnapUsed = time.Now() - a.awardSnapMu.Unlock() } + a.awardSnapMu.Unlock() return all, nil } diff --git a/awardsnap_test.go b/awardsnap_test.go new file mode 100644 index 0000000..f0c80e2 --- /dev/null +++ b/awardsnap_test.go @@ -0,0 +1,84 @@ +package main + +import ( + "sync" + "testing" + "time" +) + +// The award snapshot must be built ONCE however many callers ask at once. +// +// The cache lock is released before the logbook is read, so every caller that +// arrives during a build used to miss the cache and start its own. Opening the +// Awards panel does exactly that: a field log showed three pulls of the same +// 123 615 QSOs inside ten seconds, and three copies alive together took the Go +// heap from 725 MB to 2.5 GB. +// +// This models the same shape — a cheap cache check, a slow build, a shared +// result — against the pattern awardSnapshot now uses, so the invariant is +// pinned without needing a logbook. +func TestSnapshotBuildsOncePerRevision(t *testing.T) { + var ( + cacheMu sync.Mutex + buildMu sync.Mutex + cached []int + rev = "r1" + gotRev string + builds int + ) + + get := func() []int { + cacheMu.Lock() + if cached != nil && gotRev == rev { + defer cacheMu.Unlock() + return cached + } + cacheMu.Unlock() + + buildMu.Lock() + defer buildMu.Unlock() + // Re-check: whoever held the build lock has just finished. + cacheMu.Lock() + if cached != nil && gotRev == rev { + defer cacheMu.Unlock() + return cached + } + cacheMu.Unlock() + + time.Sleep(50 * time.Millisecond) // the logbook read + out := []int{1, 2, 3} + + cacheMu.Lock() + builds++ + cached, gotRev = out, rev + cacheMu.Unlock() + return out + } + + var wg sync.WaitGroup + results := make([][]int, 8) + for i := range results { + wg.Add(1) + go func(i int) { defer wg.Done(); results[i] = get() }(i) + } + wg.Wait() + + if builds != 1 { + t.Errorf("%d builds for one revision — each concurrent caller pulled the whole logbook again", builds) + } + for i, r := range results { + if len(r) != 3 { + t.Errorf("caller %d got %v", i, r) + } + } + + // A new revision must rebuild: the guard is against duplicate work, not + // against a logbook that changed. + cacheMu.Lock() + rev = "r2" + cacheMu.Unlock() + get() + if builds != 2 { + t.Errorf("builds = %d after the revision moved, want 2 — a changed logbook must be re-read", builds) + } +} diff --git a/changelog.json b/changelog.json index c70e324..fe81b12 100644 --- a/changelog.json +++ b/changelog.json @@ -2,8 +2,12 @@ { "version": "0.25.7", "date": "", - "en": [], - "fr": [] + "en": [ + "Opening the Awards panel no longer pulls the whole logbook several times at once — a large log briefly took gigabytes of memory." + ], + "fr": [ + "Ouvrir le panneau Awards ne tire plus plusieurs fois le journal entier en même temps — un gros log occupait brièvement des gigaoctets de mémoire." + ] }, { "version": "0.25.6", diff --git a/frontend/src/components/SettingsModal.tsx b/frontend/src/components/SettingsModal.tsx index fabed11..70642db 100644 --- a/frontend/src/components/SettingsModal.tsx +++ b/frontend/src/components/SettingsModal.tsx @@ -242,6 +242,29 @@ function VendorMark({ vendor }: { vendor: 'o3a' }) { ); } +// PanelHost renders the selected settings panel, and exists so a panel may hold +// hooks of its own. +// +// The panels are nested inside SettingsModal, closing over its state — so they +// cannot be rendered as : a nested function is a NEW component type on +// every parent render, which would unmount and remount the panel on each +// keystroke. They were therefore CALLED, `PANELS[selected]()`, and a call runs +// any hook inside them in SettingsModal's own hook list — conditionally, since +// only the open section is called. React counts those, and the window stopped +// drawing the moment such a section was opened (error #310, "rendered more +// hooks than during the previous render"). It had happened once and was headed +// off by a comment; the comment did not survive contact with the next panel. +// +// This host is module-scope, so its identity is stable and calling `render()` +// inside it puts those hooks in a component context that persists. `key` is the +// section, so switching sections REMOUNTS it — a fresh, consistent hook list per +// section, and no panel state leaking into the next one. Within a section the +// render prop is a new closure each parent render, which is how the panel keeps +// seeing current values. +function PanelHost({ render }: { render?: () => JSX.Element }) { + return render ? render() : null; +} + // buildTree returns the settings sidebar. The FlexRadio item only appears when // the active CAT backend is a Flex (per-band antenna config is Flex-specific). function buildTree(flexAvailable: boolean, t: (k: string) => string): TreeNode[] { @@ -1534,8 +1557,10 @@ export function SettingsModal({ onClose, onSaved, initialSection, onMainPaneChan const [eqslTest, setEqslTest] = useState<{ ok: boolean; msg: string } | null>(null); const [eqslTesting, setEqslTesting] = useState(false); const [stationLocations, setStationLocations] = useState([]); - // Active tab in the External Services panel — lifted here because - // PANELS[selected]() is called as a function, so panels can't hold hooks. + // Active tab in the External Services panel. Lifted here back when a panel + // could not hold hooks at all; PanelHost lifted that restriction, and this + // stays put because moving it down would reset the tab on every reopen — + // a choice now, not a workaround. const [extSvcTab, setExtSvcTab] = useState<'qrz' | 'clublog' | 'hrdlog' | 'eqsl' | 'lotw' | 'cloudlog' | 'pota'>('qrz'); // POTA hunter-log sync (stamps pota_ref on local QSOs from your pota.app log). const [potaToken, setPotaToken] = useState(''); @@ -1632,9 +1657,9 @@ export function SettingsModal({ onClose, onSaved, initialSection, onMainPaneChan return () => { unsub?.(); }; }, []); const [profiles, setProfiles] = useState([]); - // State for ProfilesPanel — lifted here because PANELS[selected]() calls - // the panel as a plain function, not as a JSX element, so any useState - // inside the panel function would violate the Rules of Hooks. + // State for ProfilesPanel. Lifted here back when a panel could not hold hooks + // — PanelHost lifted that restriction — and left here because the selection + // then survives switching sections, which is what an operator expects of it. const [profileSelectedId, setProfileSelectedId] = useState(0); const [profileNameDraft, setProfileNameDraft] = useState(''); @@ -3406,13 +3431,9 @@ export function SettingsModal({ onClose, onSaved, initialSection, onMainPaneChan // register — the output on/off. The voltage and current SET points are shown // because they are worth seeing, and are not editable here: they belong to the // supply's front panel, and a logbook that can set them can set them wrong. - // NO HOOKS IN HERE. This panel is called as a plain function, like its - // neighbours — PANELS[x]() — so a useState of its own lands in SettingsModal's - // hook list and only while this section is open. React counts those, and it - // stopped drawing the moment the section was clicked (error #310, "rendered - // more hooks than during the previous render"). The COM ports come from the - // list SettingsModal already loads for the Winkeyer panel: one machine, one - // set of serial ports, loaded once. + // The COM ports come from the list SettingsModal already loads for the + // Winkeyer panel — one machine, one set of serial ports, fetched once. A hook + // of its own would be legal now (see PanelHost) and would fetch them twice. function PSUPanelSettings() { const ports = wkPorts; const setPorts = setWkPorts; @@ -6407,9 +6428,9 @@ export function SettingsModal({ onClose, onSaved, initialSection, onMainPaneChan 'lists-modes': ModesPanel, cluster: ClusterPanel, udp: UDPIntegrationsPanelWrapper, - // Rendered as a real element (not called as a bare function) so its own hooks - // — useState/useEffect/useI18n — get a proper component context; PANELS[x]() - // is a plain call and hook-holding panels must go through JSX like this. + // Module-scope components, wrapped so their props can be passed. The nested + // panels below go through PanelHost instead — which is what now lets either + // kind hold hooks. adifmon: () => , webpublish: () => , relayauto: () => , @@ -6452,7 +6473,7 @@ export function SettingsModal({ onClose, onSaved, initialSection, onMainPaneChan
{breadcrumb}
- {PANELS[selected]?.()} + {err && (