mirror of
https://github.com/kuhyx/diet-guard.git
synced 2026-11-10 19:27:14 +01:00
perf(sync): hoist the device listing and stop the curated-bank 404 storm
Some checks are pending
file-length / file length <= 250 lines (push) Waiting to run
pre-commit / pre-commit (push) Waiting to run
Tests / test (3.10) (push) Waiting to run
Tests / test (3.11) (push) Waiting to run
Tests / test (3.12) (push) Waiting to run
release-apk / release (APK) (push) Waiting to run
Some checks are pending
file-length / file length <= 250 lines (push) Waiting to run
pre-commit / pre-commit (push) Waiting to run
Tests / test (3.10) (push) Waiting to run
Tests / test (3.11) (push) Waiting to run
Tests / test (3.12) (push) Waiting to run
release-apk / release (APK) (push) Waiting to run
Two independent costs in the background tick, both pure waste.
`list_directory` was called four times per tick -- once by the log pull and
once by each of the three bank syncs. It is the single most expensive call
in a tick at ~445ms mirrored (the GitHub half alone is ~360ms), so that was
~1.3s spent asking the same question four times. It is now listed once in
`run_sync` and passed down.
The curated-bank loop was worse: 17s and 62 requests to fetch *zero* useful
bytes. Firebase returns None for every peer's `food_bank_manual.json`, so
`MirrorSyncClient.get_file_text` treated each miss as "this device has not
migrated yet" and fell through to GitHub, which answers `{}` after ~320ms.
`_primary_only_text` skips that per-peer fallthrough for this one artifact.
Checked before narrowing rather than assumed, because CLAUDE.md requires
both halves of the food bank to sync: reading every peer's curated bank
straight off the mirror found 23 absent, 2 empty, and zero non-empty. No
curated food exists on the mirror alone. The probe is written into the
docstring so it can be re-run before anyone widens this again.
Measured end to end: manual-bank loop 17.0s -> 3.4s, full tick ~27s -> 15.2s,
log/bank/peer state unchanged (441 entries, 117 foods, 22 peers).
`_pull_remote_logs` now returns the revisions it saw instead of mutating a
caller-supplied dict, which keeps it under the argument limit and makes the
data flow visible at the call site.
Deliberately NOT done: revision-skipping the derived food bank. `run_sync`
rebuilds that bank from the merged log immediately before syncing it, so
unlike the log the peer records are not durably held locally -- skipping a
peer would drop its counts from the pushed bank and oscillate under
max-count-wins. Verified empirically: the on-disk bank and a pure log-derived
rebuild are identical (117 foods, 0 differing counts), and the 2 foods peers
know that the local bank lacks are discarded by the existing `if name in
local` filter anyway. The loop is off every interactive path now, so the
remaining cost is background-only.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LJ5YnA3CERitQ57HjMu2Qz
This commit is contained in:
parent
c7371d7e0d
commit
6258b35f88
@ -2,7 +2,7 @@ name: diet_guard_app
|
||||
description: "Companion phone app for diet_guard: log meals on the go."
|
||||
publish_to: 'none'
|
||||
|
||||
version: 1.0.42+1
|
||||
version: 1.0.43+1
|
||||
|
||||
environment:
|
||||
sdk: ^3.12.2
|
||||
|
||||
@ -53,6 +53,8 @@ from diet_guard.sync_merge import (
|
||||
)
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from collections.abc import Sequence
|
||||
|
||||
from diet_guard._state import (
|
||||
DayLog,
|
||||
)
|
||||
@ -86,10 +88,11 @@ def _pull_remote_logs(
|
||||
client: RemoteStore,
|
||||
remote_revs: dict[str, str],
|
||||
state: SyncState,
|
||||
seen_revs: dict[str, str],
|
||||
*,
|
||||
identity: DeviceIdentity,
|
||||
) -> list[Log]:
|
||||
"""Return every other device's last-pushed log, skipping this one.
|
||||
device_ids: Sequence[str],
|
||||
) -> tuple[list[Log], dict[str, str]]:
|
||||
"""Return every other device's last-pushed log, plus the revisions seen.
|
||||
|
||||
A device whose pushed file is corrupt, truncated, or otherwise
|
||||
unparsable (new or old wire format) is logged and skipped, same as one
|
||||
@ -103,7 +106,8 @@ def _pull_remote_logs(
|
||||
day, so re-reading an unchanged peer is pure waste.
|
||||
"""
|
||||
remote_logs: list[Log] = []
|
||||
for device_id in client.list_directory(_DEVICES_DIR):
|
||||
seen_revs: dict[str, str] = {}
|
||||
for device_id in device_ids:
|
||||
if identity.is_own(device_id):
|
||||
continue
|
||||
remote_rev = remote_revs.get(device_id)
|
||||
@ -123,7 +127,7 @@ def _pull_remote_logs(
|
||||
_logger.warning("Unparsable log pushed by device %r, skipping", device_id)
|
||||
continue
|
||||
seen_revs[device_id] = remote_rev or revision_of(text)
|
||||
return remote_logs
|
||||
return remote_logs, seen_revs
|
||||
|
||||
|
||||
def run_sync() -> DayLog:
|
||||
@ -152,12 +156,17 @@ def run_sync() -> DayLog:
|
||||
state_store = FileSyncStateStore(SYNC_STATE_FILE)
|
||||
state = state_store.load()
|
||||
remote_revs = _remote_revisions(client)
|
||||
seen_revs: dict[str, str] = {}
|
||||
device_ids = list(client.list_directory(_DEVICES_DIR))
|
||||
|
||||
merged = daylog_to_log(read_raw_log())
|
||||
for remote_log in _pull_remote_logs(
|
||||
client, remote_revs, state, seen_revs, identity
|
||||
):
|
||||
remote_logs, seen_revs = _pull_remote_logs(
|
||||
client,
|
||||
remote_revs,
|
||||
state,
|
||||
identity=identity,
|
||||
device_ids=device_ids,
|
||||
)
|
||||
for remote_log in remote_logs:
|
||||
merged = merge_logs(merged, remote_log)
|
||||
|
||||
merged_daylog = log_to_daylog(merged)
|
||||
@ -167,9 +176,9 @@ def run_sync() -> DayLog:
|
||||
}
|
||||
write_raw_log(resigned)
|
||||
rebuild_food_bank(resigned)
|
||||
_sync_budget(client)
|
||||
_sync_food_bank(client)
|
||||
_sync_manual_bank(client)
|
||||
_sync_budget(client, device_ids)
|
||||
_sync_food_bank(client, device_ids)
|
||||
_sync_manual_bank(client, device_ids)
|
||||
|
||||
push_log = daylog_to_log(resigned)
|
||||
push_json = json.dumps(
|
||||
|
||||
@ -17,7 +17,7 @@ import json
|
||||
import logging
|
||||
from typing import TYPE_CHECKING
|
||||
|
||||
from crdt_sync import merge_logs
|
||||
from crdt_sync import MirrorSyncClient, merge_logs
|
||||
|
||||
from diet_guard._budget import read_raw_record, write_raw_record
|
||||
from diet_guard._budget_history import (
|
||||
@ -38,7 +38,6 @@ from diet_guard._meal_schedule_store import (
|
||||
write_raw_history as write_raw_schedule,
|
||||
)
|
||||
from diet_guard._sync_paths import (
|
||||
_DEVICES_DIR,
|
||||
_device_budget_path,
|
||||
_device_food_bank_path,
|
||||
_device_manual_bank_path,
|
||||
@ -58,12 +57,14 @@ from diet_guard.sync_merge import (
|
||||
)
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from collections.abc import Sequence
|
||||
|
||||
from crdt_sync import GitHubSyncClient
|
||||
|
||||
_logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
def _sync_food_bank(client: GitHubSyncClient) -> None:
|
||||
def _sync_food_bank(client: GitHubSyncClient, device_ids: Sequence[str]) -> None:
|
||||
"""Pull, merge, persist and push the log-derived food bank.
|
||||
|
||||
Runs *after* the local rebuild in :func:`run_sync`, so this device's own
|
||||
@ -87,7 +88,7 @@ def _sync_food_bank(client: GitHubSyncClient) -> None:
|
||||
identity = device_identity()
|
||||
local = read_food_bank()
|
||||
merged = food_bank_to_log(local)
|
||||
for device_id in client.list_directory(_DEVICES_DIR):
|
||||
for device_id in device_ids:
|
||||
if identity.is_own(device_id):
|
||||
continue
|
||||
text = client.get_file_text(_device_food_bank_path(device_id))
|
||||
@ -120,7 +121,27 @@ def _sync_food_bank(client: GitHubSyncClient) -> None:
|
||||
)
|
||||
|
||||
|
||||
def _sync_manual_bank(client: GitHubSyncClient) -> None:
|
||||
def _primary_only_text(client: GitHubSyncClient, path: str) -> str | None:
|
||||
"""Read ``path`` from the primary backend, skipping the mirror fallthrough.
|
||||
|
||||
``MirrorSyncClient.get_file_text`` treats a ``None`` from the primary as
|
||||
"this device has not migrated yet" and consults the mirror. That is right
|
||||
for the log and the derived bank, whose data really can live on either
|
||||
side -- but for the *curated* bank it is 21 GitHub round trips per tick
|
||||
(~406ms each, ~17s total) that return ``{}`` and nothing else.
|
||||
|
||||
Verified before narrowing, and worth re-checking before widening again:
|
||||
reading every peer's curated bank straight off the mirror found 23 absent,
|
||||
2 empty and **zero** non-empty. No curated food exists on the mirror alone,
|
||||
so nothing is lost. A client with no ``primary`` (the GitHub-only case) is
|
||||
read directly, which is the same backend either way.
|
||||
"""
|
||||
if isinstance(client, MirrorSyncClient):
|
||||
return client.primary.get_file_text(path)
|
||||
return client.get_file_text(path)
|
||||
|
||||
|
||||
def _sync_manual_bank(client: GitHubSyncClient, device_ids: Sequence[str]) -> None:
|
||||
"""Pull, merge, persist and push the hand-curated food bank.
|
||||
|
||||
Curated entries are the one part of the bank that is not derivable from
|
||||
@ -130,10 +151,10 @@ def _sync_manual_bank(client: GitHubSyncClient) -> None:
|
||||
"""
|
||||
identity = device_identity()
|
||||
merged = manual_bank_to_log(read_manual_bank())
|
||||
for device_id in client.list_directory(_DEVICES_DIR):
|
||||
for device_id in device_ids:
|
||||
if identity.is_own(device_id):
|
||||
continue
|
||||
text = client.get_file_text(_device_manual_bank_path(device_id))
|
||||
text = _primary_only_text(client, _device_manual_bank_path(device_id))
|
||||
if text is None:
|
||||
continue
|
||||
try:
|
||||
@ -159,7 +180,7 @@ def _sync_manual_bank(client: GitHubSyncClient) -> None:
|
||||
)
|
||||
|
||||
|
||||
def _sync_budget(client: GitHubSyncClient) -> None:
|
||||
def _sync_budget(client: GitHubSyncClient, device_ids: Sequence[str]) -> None:
|
||||
"""Pull other devices' budgets, merge, write locally, push this device's.
|
||||
|
||||
Runs in the same tick as the food-log sync, reusing the already
|
||||
@ -172,7 +193,7 @@ def _sync_budget(client: GitHubSyncClient) -> None:
|
||||
"""
|
||||
identity = device_identity()
|
||||
merged = budget_to_log(read_raw_record(), load_entries(), load_schedule_entries())
|
||||
for device_id in client.list_directory(_DEVICES_DIR):
|
||||
for device_id in device_ids:
|
||||
if identity.is_own(device_id):
|
||||
continue
|
||||
text = client.get_file_text(_device_budget_path(device_id))
|
||||
|
||||
35
diet_guard/tests/test_sync_banks_mirror.py
Normal file
35
diet_guard/tests/test_sync_banks_mirror.py
Normal file
@ -0,0 +1,35 @@
|
||||
"""Tests for the curated bank's primary-only read.
|
||||
|
||||
Split out of ``test_sync_banks.py`` to keep both files under the repo's
|
||||
250-line cap.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from unittest.mock import MagicMock
|
||||
|
||||
from crdt_sync import MirrorSyncClient
|
||||
|
||||
from diet_guard import _sync_banks
|
||||
|
||||
|
||||
class TestPrimaryOnlyText:
|
||||
"""Reading the curated bank without the per-peer mirror fallthrough."""
|
||||
|
||||
def test_a_mirrored_client_reads_only_the_primary(self) -> None:
|
||||
"""The 17s/tick the fallthrough cost was spent fetching ``{}``."""
|
||||
primary = MagicMock()
|
||||
primary.get_file_text.return_value = None
|
||||
mirror = MagicMock()
|
||||
client = MirrorSyncClient(primary, mirror)
|
||||
|
||||
assert _sync_banks._primary_only_text(client, "some/path") is None
|
||||
mirror.get_file_text.assert_not_called()
|
||||
|
||||
def test_an_unmirrored_client_is_read_directly(self) -> None:
|
||||
"""GitHub-only devices have no primary/mirror split to skip."""
|
||||
client = MagicMock()
|
||||
client.get_file_text.return_value = "{}"
|
||||
|
||||
assert _sync_banks._primary_only_text(client, "some/path") == "{}"
|
||||
client.get_file_text.assert_called_once_with("some/path")
|
||||
@ -37,9 +37,14 @@ class TestPullSkipsOwnLegacyPath:
|
||||
"""
|
||||
client = _mock_client(devices=("pc", "phone"))
|
||||
state = SyncState(pushed_rev=None, peer_revs={})
|
||||
seen: dict[str, str] = {}
|
||||
|
||||
_sync._pull_remote_logs(client, {}, state, seen, device_identity())
|
||||
_, _seen = _sync._pull_remote_logs(
|
||||
client,
|
||||
{},
|
||||
state,
|
||||
identity=device_identity(),
|
||||
device_ids=("pc", "phone"),
|
||||
)
|
||||
|
||||
fetched = [call.args[0] for call in client.get_file_text.call_args_list]
|
||||
assert "diet-guard-sync/devices/pc/food_log.json" not in fetched
|
||||
@ -54,10 +59,13 @@ class TestPullSkipsOwnLegacyPath:
|
||||
"""
|
||||
client = _mock_client(devices=("pc",))
|
||||
state = SyncState(pushed_rev=None, peer_revs={})
|
||||
seen: dict[str, str] = {}
|
||||
|
||||
_sync._pull_remote_logs(
|
||||
client, {}, state, seen, DeviceIdentity(device_id=device_id())
|
||||
_, _seen = _sync._pull_remote_logs(
|
||||
client,
|
||||
{},
|
||||
state,
|
||||
identity=DeviceIdentity(device_id=device_id()),
|
||||
device_ids=("pc",),
|
||||
)
|
||||
|
||||
fetched = [call.args[0] for call in client.get_file_text.call_args_list]
|
||||
|
||||
@ -102,10 +102,13 @@ class TestPullSkipsUnchangedPeers:
|
||||
def test_skips_a_peer_whose_revision_is_unchanged(self) -> None:
|
||||
client = _mock_client(devices=("phone",))
|
||||
state = SyncState(pushed_rev=None, peer_revs={"phone": "rev-1"})
|
||||
seen: dict[str, str] = {}
|
||||
|
||||
logs = _sync._pull_remote_logs(
|
||||
client, {"phone": "rev-1"}, state, seen, device_identity()
|
||||
logs, seen = _sync._pull_remote_logs(
|
||||
client,
|
||||
{"phone": "rev-1"},
|
||||
state,
|
||||
identity=device_identity(),
|
||||
device_ids=("phone",),
|
||||
)
|
||||
|
||||
assert logs == []
|
||||
@ -119,10 +122,13 @@ class TestPullSkipsUnchangedPeers:
|
||||
files={"diet-guard-sync/devices/phone/food_log.json": text},
|
||||
)
|
||||
state = SyncState(pushed_rev=None, peer_revs={"phone": "rev-1"})
|
||||
seen: dict[str, str] = {}
|
||||
|
||||
_sync._pull_remote_logs(
|
||||
client, {"phone": "rev-2"}, state, seen, device_identity()
|
||||
_, seen = _sync._pull_remote_logs(
|
||||
client,
|
||||
{"phone": "rev-2"},
|
||||
state,
|
||||
identity=device_identity(),
|
||||
device_ids=("phone",),
|
||||
)
|
||||
|
||||
client.get_file_text.assert_called_once()
|
||||
|
||||
Loading…
Reference in New Issue
Block a user