feat(data-structures/unstable): add RollingCounter serialization#7074
feat(data-structures/unstable): add RollingCounter serialization#7074tomas-zijdemans wants to merge 5 commits intodenoland:mainfrom
RollingCounter serialization#7074Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7074 +/- ##
==========================================
+ Coverage 94.41% 94.43% +0.02%
==========================================
Files 630 630
Lines 50490 50601 +111
Branches 8949 8978 +29
==========================================
+ Hits 47669 47785 +116
+ Misses 2249 2247 -2
+ Partials 572 569 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Made-with: Cursor
bartlomieju
left a comment
There was a problem hiding this comment.
The rotate() optimization: could you share the benchmarks that motivated this rewrite? The original 4-line loop was simple and correct — the new version is ~25 lines with wrap-around logic and already introduced a cursor corruption bug (rotate(0)).
Modulo per iteration is cheap on modern engines, so I'd want to see concrete numbers before trading readability for this. What was the benchmark setup (segment count, step sizes) and what workload are you optimizing for?
If the gains are significant for your use case, this might be better as a separate PR with the benchmarks included so the tradeoff is documented.
|
Below is the benchmark. The speedup is small for regular rotation (the most common use case). I'm seeing an average of about 10% improvement across sizes. The big winner is, the arguably lot less used, bulk rotation seeing 5.5x to 7.5x. I will revert the perf changes, and then we can see if that's something we want to introduce in a future PR. The readability hit is real, fully agree on that! |
toJSON()/RollingCounter.from()for JSON-compatible serialization and restorationcurrentgetter for reading the newest segment without iterationSymbol.toStringTagto match module precedent.