Skip to content

stats: avoid blocking updates while loading history - #8541

Open
Sil3ntVip3r wants to merge 3 commits into
AdguardTeam:masterfrom
Sil3ntVip3r:codex/3113-stats-io-profile
Open

stats: avoid blocking updates while loading history#8541
Sil3ntVip3r wants to merge 3 commits into
AdguardTeam:masterfrom
Sil3ntVip3r:codex/3113-stats-io-profile

Conversation

@Sil3ntVip3r

Copy link
Copy Markdown

Summary

  • snapshot the current in-memory statistics unit and a read-only bbolt view
    under a short lock
  • release the DNS-path update lock before loading and decoding historical units
  • preserve an exact boundary across concurrent hourly flushes so the current
    unit is neither lost nor counted twice
  • add persisted-history benchmarks for 24 hours through 90 days

Evidence

On current unmodified master, both new concurrency regressions fail because
loadUnits keeps currMu held while opening/loading a writable transaction:

TestStatsCtx_LoadUnitsDoesNotBlockUpdate: did not receive after 1s
TestStatsCtx_LoadUnitsConcurrentFlush: did not receive after 1s

With this change, both tests pass 20 consecutive runs under -race. The
90-day persisted-history benchmark on an Apple M4 Pro measured approximately
74 ms for loadUnits and 90 ms for getData; the important behavioral change
is that those reads no longer hold the update lock for that duration.

Testing

  • focused concurrency regressions, 20 times under -race
  • full internal/stats package under -race
  • persisted-history benchmarks at 24, 168, 720, and 2,160 hours
  • make go-check
  • git diff --check

This addresses the confirmed DNS-path blocking behavior in #3113 without
closing the broader storage/schema investigation.

@Sil3ntVip3r

Copy link
Copy Markdown
Author

Updated against current master (b2e25729) with a normal merge; the only conflict was the changelog, and both entries are retained.

While revalidating the integration, I found and fixed one remaining blocking path in this PR’s scope: loadUnits previously held its bbolt read transaction while decoding all stored units. A map-growing hourly flush can then wait for that read transaction while holding currMu, delaying synchronous stats updates on the DNS path. The updated implementation copies the consistent stored-unit snapshot, closes the read transaction, and only then decodes it.

The new regression forces actual stats-database growth, pauses decoding, and verifies that Update completes before decoding is released. It fails on the prior PR head and passes on exact head 4bd346085a66e05084d53e0d8ac6ebcd2702e1c1.

Validation:

  • all three concurrency regressions, 20 times under -race;
  • go test -race -count=1 ./internal/stats ./internal/dnsforward;
  • repository Go checks, text/Markdown linting, whitespace checks, and the repository commit hook’s race-enabled suite;
  • local 90-day one-iteration loader benchmark remained comparable to the prior head (79.1 ms vs. 79.8 ms).

GitHub now reports this PR as mergeable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant