perf: use non-crypto RNG for internal IDs + add benchmark harness#33
Open
danReynolds wants to merge 2 commits into
Open
perf: use non-crypto RNG for internal IDs + add benchmark harness#33danReynolds wants to merge 2 commits into
danReynolds wants to merge 2 commits into
Conversation
Covers write/read throughput, ID generation, broadcast latency as a function of observer count, and sorted-query rebroadcast cost as a function of result-set size. Lives under benchmark/ (excluded from the test suite and CI); run with `flutter test benchmark/loon_benchmark.dart`.
Observer IDs and worker request-correlation IDs only need process-local uniqueness, but both were drawn from the OS CSPRNG via Random.secure(). Add generateInternalId backed by a plain Random and use it for those two call sites; document IDs keep the secure generator since apps may rely on their unguessability. Microbenchmark: ID generation drops from ~2.85us to ~0.25us/op (~11x). The end-to-end subscription win is smaller (~6%) since setup is dominated by stream-controller and store costs, but the change is free and removes needless CSPRNG overhead from per-subscription and per-message paths.
There was a problem hiding this comment.
Pull request overview
Improves performance on hot paths by avoiding OS CSPRNG overhead for IDs that only require process-local uniqueness, and adds a standalone benchmark harness to measure key Loon operations.
Changes:
- Add
generateInternalId()backed by non-cryptographicRandom()and refactor ID generation into a shared helper. - Switch broadcast observer IDs and persistor worker request-correlation IDs from
generateId()(secure) togenerateInternalId()(fast). - Add
benchmark/loon_benchmark.dartfor manual performance measurement viaflutter test benchmark/loon_benchmark.dart.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| lib/utils/id.dart | Adds a fast internal-ID generator while keeping secure IDs for externally visible document IDs. |
| lib/persistor/worker/messages.dart | Uses fast internal IDs for worker request/response correlation IDs. |
| lib/broadcast_observer.dart | Uses fast internal IDs for observer instance identifiers. |
| benchmark/loon_benchmark.dart | Introduces a manual benchmark harness covering write/read, ID generation, subscriptions, broadcast scaling, and query rebroadcast cost. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+82
to
+86
| final subs = <dynamic>[]; | ||
| final sw = Stopwatch()..start(); | ||
| for (var i = 0; i < n; i++) { | ||
| subs.add(col.doc('doc_$i').observe().stream().listen((_) {})); | ||
| } |
Comment on lines
+15
to
+18
| setUp(() async { | ||
| await Loon.clearAll(); | ||
| Loon.unsubscribe(); | ||
| }); |
Comment on lines
+20
to
+23
| tearDownAll(() async { | ||
| await Loon.clearAll(); | ||
| Loon.unsubscribe(); | ||
| }); |
Comment on lines
+105
to
+107
| for (final n in [0, 100, 1000, 5000]) { | ||
| await Loon.clearAll(); | ||
| Loon.unsubscribe(); |
Comment on lines
+157
to
+159
| for (final m in [100, 1000, 10000]) { | ||
| await Loon.clearAll(); | ||
| Loon.unsubscribe(); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two related changes from a performance audit:
perf: use non-crypto RNG for internal IDs— Observer IDs (broadcast_observer.dart) and worker request-correlation IDs (persistor/worker/messages.dart) only need process-local uniqueness, but both were drawn from the OS CSPRNG viaRandom.secure(). This addsgenerateInternalIdbacked by a plainRandomand uses it for those two call sites. Document IDs keepgenerateId(secure) since applications may rely on their unguessability.Add performance benchmark harness— A standalone harness underbenchmark/(excluded from the test suite and CI) covering write/read throughput, ID generation, subscription setup, broadcast latency vs. observer count, and sorted-query rebroadcast cost vs. result-set size. Run withflutter test benchmark/loon_benchmark.dart.Why
The harness surfaced that
generateIdcosts ~2.85µs/op — about 60% of a full document write — because of the CSPRNG, and it's paid on every observer subscription. That's needless: observer/worker IDs don't need cryptographic unguessability.Measured impact (this machine)
The end-to-end subscription win is smaller (~6%) because subscription setup is dominated by stream-controller allocation and store writes, not ID generation. The change is free and zero-risk, and it removes the CSPRNG from the per-subscription and per-worker-message paths.
Test plan
flutter test test/core --concurrency=1— 106/106 greenflutter analyzecleanflutter test benchmark/loon_benchmark.dartruns and reportsGenerated by Claude Code