[v2] Perf: Use one atom subscription per Field - #2358
Conversation
Each React Field subscribed to the same atom once for value and again for meta. The field atom already preserves state identity when neither changes, so one whole-state subscription has the same render behavior. Co-Authored-By: OpenAI Codex <noreply@openai.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe field subscription now observes the complete field state through one subscription. The field-context test description and expected active subscription count reflect this change. ChangesField subscription update
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change reduces each field from two atom subscriptions to one without supplied evidence of altered render behavior or other merge-blocking impact; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description clearly explains the subscription reduction and its motivation, but it omits the required "## 🎯 Changes", "## ✅ Checklist", and "## 🚀 Release Impact" sections, including checklist and changeset status.
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
You mention performance for the PR. Do you have some rough numbers for it? Memory / runtime? |
|
@LeCarbonator all the perf is just from going from 2 to 1 I ran a focused benchmark using the exact before/after hooks with production React 19.2.8. Each variant ran in a fresh process 10 times, alternating order; results are medians. 1,000 mounted fields
The harness mounts non-array fields backed by individual TanStack Store atoms, measures synchronous mount/update work, and measures retained heap after forced GC. This isolates subscription overhead rather than measuring a complete application, so the absolute timings are directional. The consistent result is half the subscriptions and callbacks, with roughly 3.2 KiB less retained heap per mounted field. |
|
Sorry, don't have a ton of cycles here to respond more in depth, but I'd attempted this in v1 and benchmarks showed MASSIVE improvements then (like 10x), but the UX of 1000 mounted fields slowed to a crawl for reasons I couldn't identify. Might be helpful for us to all sit down and discuss and on our end manually test. Not saying that this is a bad PR or approach, just flagging that benchmarks for this has been directionally wrong before. |
|
Let sol on ultra try to figure out what you were referring to, obviously feel free to review/close whatever. Model used: OpenAI Codex — GPT-5.6 Sol Codex investigated the relevant v1 history and benchmarked the exact base and PR commits in production Chromium. The closest v1 match is PR #2036. That implementation replaced seven subscriptions with one selector that created a new seven-property object and used a custom comparator. It was also mixed with substantial core store and notification changes, so the historical results do not isolate subscription consolidation as the cause. Its “1,000-field” benchmarks actually rendered 100 fields in jsdom and did not measure browser rendering or memory. V2 has a different design. Before this PR, a normal field subscribes separately to if (prev?.meta === meta && prev.value === value) {
return prev
}Therefore, the rerender conditions are equivalent: The PR does not create a projected object, add a custom comparator, or change core notification behavior. Real-browser confirmation with 1,000 controlled fields and 60 fresh contexts per variant found:
|
|
@scttcper I'm an idiot and I wasted your tokens. I just reviewed the diff quickly. For some reason I'd gotten a LOT wrong in my mind about what this PR was and assumed it was doing something more closely related to splitting one store into many for modularity. Frankly I don't know how I got things this mixed up. So sorry. This PR LGTM broadly, but might have weird consequences with Compiler. So long as it works well there we should merge. Sorry again, that's embarrassing. |
|
@crutchcorn work pays for it 🤷 |
|
I think this should be compiler safe. Don't see how changing it from two to one could have consequences for it. I wish I had better Wi-Fi to actually try this, but that'll have to wait until next monday. |
|
View your CI Pipeline Execution ↗ for commit 2257d3d
☁️ Nx Cloud last updated this comment at |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #2358 +/- ##
========================================
Coverage ? 92.91%
========================================
Files ? 14
Lines ? 240
Branches ? 19
========================================
Hits ? 223
Misses ? 16
Partials ? 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4 participants
Each React
Fieldsubscribed to the same atom twice, once for value and once for meta. The field atom already preserves state identity when neither changes, so use one whole-state subscription instead.This cuts active subscriptions from two to one per field with the same render behavior.
Summary by CodeRabbit
Bug Fixes
Tests