enhance(normalizr): Defer entity table POJO construction during normalize#3881
Closed
enhance(normalizr): Defer entity table POJO construction during normalize#3881
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
🦋 Changeset detectedLatest commit: 131a395 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
Size Change: +164 B (+0.2%) Total Size: 80.8 kB 📦 View Changed
ℹ️ View Unchanged
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Benchmark React
Details
| Benchmark suite | Current: 131a395 | Previous: cc330d6 | Ratio |
|---|---|---|---|
data-client: getlist-100 |
143.89 ops/s (± 5.3%) |
142.86 ops/s (± 5.9%) |
0.99 |
data-client: getlist-500 |
43.53 ops/s (± 5.5%) |
38.24 ops/s (± 7.1%) |
0.88 |
data-client: update-entity |
384.62 ops/s (± 11.2%) |
344.83 ops/s (± 9.0%) |
0.90 |
data-client: update-user |
400.64 ops/s (± 8.1%) |
384.62 ops/s (± 7.9%) |
0.96 |
data-client: getlist-500-sorted |
37.81 ops/s (± 7.4%) |
42.74 ops/s (± 7.0%) |
1.13 |
data-client: update-entity-sorted |
333.33 ops/s (± 8.6%) |
322.58 ops/s (± 8.3%) |
0.97 |
data-client: update-entity-multi-view |
357.14 ops/s (± 7.1%) |
357.14 ops/s (± 7.8%) |
1 |
data-client: list-detail-switch-10 |
6.62 ops/s (± 4.9%) |
7.88 ops/s (± 7.4%) |
1.19 |
data-client: update-user-10000 |
94.79 ops/s (± 12.3%) |
81.3 ops/s (± 15.4%) |
0.86 |
data-client: invalidate-and-resolve |
38.47 ops/s (± 3.8%) |
35.59 ops/s (± 4.4%) |
0.93 |
data-client: unshift-item |
246.95 ops/s (± 3.7%) |
227.27 ops/s (± 5.1%) |
0.92 |
data-client: delete-item |
277.78 ops/s (± 6.4%) |
285.71 ops/s (± 6.6%) |
1.03 |
data-client: move-item |
204.08 ops/s (± 6.3%) |
196.08 ops/s (± 5.9%) |
0.96 |
This comment was automatically generated by workflow using github-action-benchmark.
Contributor
There was a problem hiding this comment.
Benchmark
Details
| Benchmark suite | Current: 131a395 | Previous: cc330d6 | Ratio |
|---|---|---|---|
normalizeLong |
428 ops/sec (±1.23%) |
438 ops/sec (±2.01%) |
1.02 |
normalizeLong Values |
375 ops/sec (±0.39%) |
409 ops/sec (±1.30%) |
1.09 |
denormalizeLong |
277 ops/sec (±5.22%) |
249 ops/sec (±4.26%) |
0.90 |
denormalizeLong Values |
343 ops/sec (±0.48%) |
226 ops/sec (±3.37%) |
0.66 |
denormalizeLong donotcache |
1002 ops/sec (±0.13%) |
1035 ops/sec (±0.71%) |
1.03 |
denormalizeLong Values donotcache |
755 ops/sec (±0.18%) |
751 ops/sec (±0.14%) |
0.99 |
denormalizeShort donotcache 500x |
1568 ops/sec (±0.13%) |
1572 ops/sec (±0.21%) |
1.00 |
denormalizeShort 500x |
1063 ops/sec (±2.06%) |
745 ops/sec (±3.28%) |
0.70 |
denormalizeShort 500x withCache |
7364 ops/sec (±0.15%) |
7527 ops/sec (±0.25%) |
1.02 |
queryShort 500x withCache |
3108 ops/sec (±0.06%) |
2974 ops/sec (±0.10%) |
0.96 |
buildQueryKey All |
53418 ops/sec (±0.44%) |
56343 ops/sec (±0.45%) |
1.05 |
query All withCache |
5915 ops/sec (±0.24%) |
6654 ops/sec (±0.35%) |
1.12 |
denormalizeLong with mixin Entity |
357 ops/sec (±2.53%) |
231 ops/sec (±4.16%) |
0.65 |
denormalizeLong withCache |
7495 ops/sec (±0.21%) |
7143 ops/sec (±0.22%) |
0.95 |
denormalizeLong Values withCache |
5419 ops/sec (±0.17%) |
4991 ops/sec (±0.67%) |
0.92 |
denormalizeLong All withCache |
5771 ops/sec (±0.19%) |
6369 ops/sec (±0.31%) |
1.10 |
denormalizeLong Query-sorted withCache |
5971 ops/sec (±0.16%) |
6738 ops/sec (±0.23%) |
1.13 |
denormalizeLongAndShort withEntityCacheOnly |
1798 ops/sec (±0.23%) |
1819 ops/sec (±0.20%) |
1.01 |
denormalize bidirectional 50 |
6971 ops/sec (±0.26%) |
5227 ops/sec (±4.07%) |
0.75 |
denormalize bidirectional 50 donotcache |
40323 ops/sec (±1.25%) |
41228 ops/sec (±1.25%) |
1.02 |
getResponse |
4690 ops/sec (±0.67%) |
4534 ops/sec (±0.66%) |
0.97 |
getResponse (null) |
10129372 ops/sec (±0.65%) |
10894251 ops/sec (±0.78%) |
1.08 |
getResponse (clear cache) |
337 ops/sec (±2.70%) |
234 ops/sec (±3.72%) |
0.69 |
getSmallResponse |
3620 ops/sec (±1.06%) |
3476 ops/sec (±0.19%) |
0.96 |
getSmallInferredResponse |
2793 ops/sec (±0.19%) |
2748 ops/sec (±0.14%) |
0.98 |
getResponse Collection |
4762 ops/sec (±0.86%) |
4471 ops/sec (±0.35%) |
0.94 |
get Collection |
4675 ops/sec (±0.31%) |
4550 ops/sec (±0.77%) |
0.97 |
get Query-sorted |
4790 ops/sec (±0.24%) |
5228 ops/sec (±0.33%) |
1.09 |
setLong |
429 ops/sec (±0.51%) |
464 ops/sec (±0.20%) |
1.08 |
setLongWithMerge |
280 ops/sec (±0.15%) |
252 ops/sec (±0.50%) |
0.90 |
setLongWithSimpleMerge |
299 ops/sec (±0.11%) |
270 ops/sec (±0.24%) |
0.90 |
setSmallResponse 500x |
890 ops/sec (±1.76%) |
929 ops/sec (±0.08%) |
1.04 |
This comment was automatically generated by workflow using github-action-benchmark.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3881 +/- ##
==========================================
+ Coverage 98.11% 98.13% +0.02%
==========================================
Files 153 153
Lines 2913 2945 +32
Branches 565 573 +8
==========================================
+ Hits 2858 2890 +32
Misses 11 11
Partials 44 44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c0e07c1. Configure here.
| const keys = Object.keys(original); | ||
| for (let i = 0; i < keys.length; i++) { | ||
| const pk = keys[i]; | ||
| result[pk] = map.get(pk) ?? original[pk]; |
There was a problem hiding this comment.
Nullish coalescing loses explicit null/undefined Map entries in finalize
Low Severity
In finalize(), result[pk] = map.get(pk) ?? original[pk] uses nullish coalescing, which cannot distinguish between "pk not in map" and "pk in map with value null/undefined". If a schema's merge/mergeWithStore were to return null or undefined, the original store value would be preserved instead of being overwritten — a behavioral difference from the old code where this.entities[key][pk] = entity always stored the exact value. Using map.has(pk) ? map.get(pk) : original[pk] would be semantically equivalent to the old behavior.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit c0e07c1. Configure here.
…lize Entity tables were previously cloned upfront via spread in getNewEntities, then mutated property-by-property in _setEntity. This clone-then-mutate pattern causes V8 "field type constness changed" invalidations and a Maglev bailout on getNewEntities due to insufficient type feedback for the POJO property accesses. Restructured to accumulate entities in Maps during normalization and build final POJOs in a single finalize() pass where each property is written exactly once. This: - Eliminates the getNewEntities V8 deopt bailout (15 → 14 bailouts) - Preserves V8 field constness on entity table objects - Correctly handles index updates by looking up old entities from the in-progress Map before falling back to the original store - Defers entitiesMeta writes similarly via a newMeta Map Also refactors handleIndexes to accept the old entity directly rather than the entire store entities table, simplifying the lookup chain. Made-with: Cursor
ntucker
force-pushed
the
perf-deferred-pojo-construction
branch
from
c0e07c1 to
131a395
Compare
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
1 participant
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.
Motivation
V8 trace profiling (
examples/benchmark-reactwithBENCH_V8_TRACE=true) revealed two issues in the normalization hot path:getNewEntitiesMaglev bailout — "Insufficient type feedback for generic named access" onthis.entitiesduring the POJO clone step. The function was compiled by Maglev early but bailed out and never re-optimized, staying in interpreter mode for the rest of the benchmark.Clone-then-mutate pattern —
getNewEntitiescloned entity tables via spread ({ ...this.entities[key] }), then_setEntityoverwrote properties on the cloned POJO. When a property copied from the original (marked "const" by V8) was overwritten, V8 triggered "field type constness changed" invalidations on any compiled code depending on that object's hidden class.Solution
Defer all POJO construction until after normalization completes:
Map<string, Map<string, any>>— no POJO cloning or mutationdelegate.finalize()builds final POJOs in a single pass where each property is written exactly once (preserving V8 field constness)handleIndexesnow receives the old entity directly (looked up from the in-progress Map first, then the original store) instead of the entire store entities tablenewMetaMap, withgetMeta()checking the Map before falling back to the storeV8 trace results (React benchmark
getlist-100):getNewEntitiesbailout — now compiles to Maglev and stays optimized)getNewEntitiesgoes from: marked → compiled → bailed out → interpreter, to: marked → compiled → stays optimizedTests: 154 normalizr + 963 core/react/endpoint tests pass.
Open questions
N/A
Made with Cursor
Note
Medium Risk
Touches the core normalization hot path by changing how entities, indexes, and metadata are accumulated and materialized, which could surface subtle ordering/merge/index invalidation regressions despite being largely internal.
Overview
Normalization now defers entity/meta POJO construction until after traversal.
NormalizeDelegatestops cloning and mutatingentities/entitiesMetaduringgetNewEntities; instead it accumulates updates in per-entityMaps and adds a newfinalize()pass that merges original tables with accumulated updates by writing each property once.Index maintenance is adjusted to compute invalidations from the previous entity value (from the in-progress map or original store) before storing the new entity, and metadata updates are similarly deferred via a
newMetamap withgetMeta()consulting pending values first.normalize()now callsdelegate.finalize()before returning, and a patch changeset documents the performance motivation; the Immutable delegate is lightly refactored to use lazy init helpers for its internal maps.Reviewed by Cursor Bugbot for commit 131a395. Bugbot is set up for automated code reviews on this repo. Configure here.