Skip to content

fix: preserve sparse slots in optimized slice - #1335

Open
nk-batman21 wants to merge 2 commits into
immerjs:mainfrom
nk-batman21:fix-sparse-slice
Open

nk-batman21 wants to merge 2 commits into
immerjs:mainfrom
nk-batman21:fix-sparse-slice

Conversation

@nk-batman21

Copy link
Copy Markdown

With enableArrayMethods(), slice() fills sparse array slots with undefined. A subsequent slice().map(item => item.id) then throws on slots that native map would skip.

Preserve the empty slots while keeping selected objects as drafts. Regression tests cover full, bounded and negative-index slices, explicit undefined values, and mutations through the sliced objects.

  • Checked: four regression cases fail in plugin-enabled mode before the fix and pass afterward; native-mode cases pass in both versions.
  • Ran: yarn test (3,872 source tests and 3,321 production-build tests passed, with eight existing skips in each suite; builds and Flow passed).
  • Ran: Prettier on the implementation and added test range, and git diff --check.

@mweststrate

Copy link
Copy Markdown
Collaborator

I'm not sure whether sparse arrays are a necessarily sane concept in JavaScript, so I'd want to understand the performance ramifications first (the patch itself is small enough).

@nk-batman21

Copy link
Copy Markdown
Author

c10aba9 keeps push() for present elements and only extends length for holes, avoiding the dense-to-holey transition in V8 from the first patch. For dense produce → slice(1, -1).map(...), two warmed production-bundle runs on Node 24.18.0/V8 13.6 (Windows, Core Ultra 5 235U, one CPU, --expose-gc, 60 samples per run with all six version orders) gave these median paired time changes versus 8848a5b; all 3,872 source tests, 3,321 production tests and Flow still pass.

Dense input Auto-freeze Original 13fa897 Revised c10aba9
1,000 objects, map to IDs off +6.5% to +11.7% -3.0% to +0.1%
1,000 objects, map to IDs on +5.2% to +5.4% -0.8% to -0.3%
100 numbers, identity map off +14.8% to +16.1% +0.1% to +0.4%
1,000 numbers, identity map off +16.0% to +18.2% +0.8% to +1.8%

@markerikson

Copy link
Copy Markdown
Collaborator

@mweststrate yeah, I have very mixed feelings about all these array edge case PRs :( on the one hand I agree things "ought" to work the same as in vanilla JS, and most of the changes have been small. on the other hand... man there's a lot of edge cases and sometimes you gotta draw a line and say "nope we just don't handle that and also why would anyone do that in the first place".

@nk-batman21

Copy link
Copy Markdown
Author

The discussion in #1019 makes the scope concern clear, and I agree that JavaScript parity alone isn't enough reason to add support. This PR came from a synthetic repro, with no reported application failure behind it, so I'm fine with closing it if sparse arrays remain outside the intended scope.

@mweststrate

mweststrate commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator

@markerikson yeah I have two problems with it:

  1. it feels like death-by-a-thousand papercuts. All super micro perf regressions and code size increases, that at some point might start to add up.
  2. The other concern I have, if we find in the future some alternative way to implement this and make it super fast (e.g. we use some WASM / native data structure / JS-to-rust / whatever), all these pesky JavaScript-isms will make more creative implementations to the same problem likely impossible to execute.

I think I'm going to introduce a new label "we-can-merge-this-if-someone-actually-cares-and-provides-a-real-life-use-case"

@mweststrate

Copy link
Copy Markdown
Collaborator

@nk-batman21 thanks for elaborating. I'll add the label and can close after a while if it doesn't pop up for anyone.

@mweststrate mweststrate added can-merge-if-someone-actually-cares Valid fix, but unclear cost/benefit and may hinder future work. Can merge if someone champions it. and removed can-merge-if-someone-actually-cares Valid fix, but unclear cost/benefit and may hinder future work. Can merge if someone champions it. labels Oct 20, 2026

This branch has not been deployed

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

Labels

can-merge-if-someone-actually-cares Valid fix, but unclear cost/benefit and may hinder future work. Can merge if someone champions it.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants