perf(awards): build the logbook snapshot once, not once per caller
A field log showed three pulls of the same 123 615 QSOs inside ten seconds, the Go heap going 725 MB → 2213 MB → 2539 MB. It is released after the idle TTL, so not a leak — just the same work done three times with all three results alive at once. The cache lock was released before the logbook was read, so every caller that arrived during a build missed the cache and started its own. Opening the Awards panel does exactly that. One builder at a time now, with the usual re-check after taking the lock: the second and third caller wait out the seconds they were going to spend anyway, minus two round trips to the database. The slice is also sized from the previous build. Growing to 123 000 structs by doubling copies the whole thing a dozen times and holds the old and the new array together at each step, on the largest object OpsLog keeps. Also, the settings panel registry. The hook trap that broke the window when the power-supply panel was opened was headed off by a comment, and the comment did not survive contact with the next panel — so it is now structural. PanelHost is a module-scope component that calls the selected panel and is keyed by section: hooks inside a panel land in a component context that persists, section changes remount it so each gets a fresh and consistent hook list, and no panel state leaks into the next. Rendering the panels as <Panel /> instead — the obvious refactor — would have been wrong: they are nested inside SettingsModal and close over its state, so each parent render makes a new component TYPE and React would unmount and remount the panel on every keystroke.
This commit is contained in:
@@ -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))
|
||||
|
||||
if revErr == nil {
|
||||
a.awardSnapMu.Lock()
|
||||
a.awardSnapCap = len(all) // next build starts the right size
|
||||
if revErr == nil {
|
||||
a.awardSnap = all
|
||||
a.awardSnapRev = rev
|
||||
a.awardSnapUsed = time.Now()
|
||||
a.awardSnapMu.Unlock()
|
||||
}
|
||||
a.awardSnapMu.Unlock()
|
||||
return all, nil
|
||||
}
|
||||
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
+6
-2
@@ -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",
|
||||
|
||||
@@ -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 <Panel />: 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<string[]>([]);
|
||||
// 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<Profile[]>([]);
|
||||
// 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<number>(0);
|
||||
const [profileNameDraft, setProfileNameDraft] = useState<string>('');
|
||||
|
||||
@@ -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: () => <ADIFMonitorPanel />,
|
||||
webpublish: () => <WebPublishPanel />,
|
||||
relayauto: () => <RelayAutoPanel />,
|
||||
@@ -6452,7 +6473,7 @@ export function SettingsModal({ onClose, onSaved, initialSection, onMainPaneChan
|
||||
<div className="text-[10px] uppercase tracking-wider text-muted-foreground mb-3 font-semibold">
|
||||
{breadcrumb}
|
||||
</div>
|
||||
{PANELS[selected]?.()}
|
||||
<PanelHost key={selected} render={PANELS[selected]} />
|
||||
|
||||
{err && (
|
||||
<div className="mt-6 text-xs text-destructive bg-destructive/10 border border-destructive/30 rounded-md px-3 py-2 max-w-2xl">
|
||||
|
||||
Reference in New Issue
Block a user