Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: Support copy-on-write in TypedArrays #707

Closed
wants to merge 7 commits into from

Conversation

Gelio
Copy link
Contributor

@Gelio Gelio commented Nov 21, 2020

This is an attempt to fix #696

This MR adds support for copy-on-write for TypedArrays (e.g. Uint16Array).

It correctly handles TypedArrays with the same underlying ArrayBuffer, so when copying 2 arrays with the same underlying buffer, the drafts will also have the same underlying buffer.

I was not sure how to handle ProxyTypes and Archtypes, so feel free to suggest another way to handle those. Also, do not hesitate listing things I could have done better 馃檪

Limitations

I did not spend enough time to fix those. The known limitations/missing features are:

  1. No support for patches
  2. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray/subarray will not return a draft

Possible optimizations

  1. Not copying the array when the value is set to the same number

Testing

Aside from the added automated tests, I did not test this feature extensively.

Handles TypedArrays with the same underlying buffer,
so the buffer is copied only once.

Limitations:
* No patch support
* TypedArray.subarray does not return a draft

Possible optimizations:
1. Not copying when a value is set to the same number
@Gelio Gelio mentioned this pull request Nov 21, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 21, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 1f70360:

Sandbox Source
Immer sandbox Configuration

@Gelio
Copy link
Contributor Author

Gelio commented Nov 22, 2020

I have just noticed a mistake: instanceof and ArrayBuffer.isView will not work on the draft.

I'll add a test to ensure that those work and try to fix those

@Gelio
Copy link
Contributor Author

Gelio commented Dec 2, 2020

I have added a few more tests and commits, and changed the approach for what the proxy target should be in the case of typed arrays. I believe it should behave as expected

I'm looking forward to receiving feedback 馃檪

@mweststrate
Copy link
Collaborator

Sorry, currently not having a lot of bandwidth. Since this is quite a meaty change, probably will only review it in January or something, hope that isn't too much of a problem.

@Gelio
Copy link
Contributor Author

Gelio commented Dec 7, 2020

@mweststrate Sure, no problem 馃檪 Take your time

@mweststrate
Copy link
Collaborator

Closing as this feature never landed

@mweststrate mweststrate closed this Jan 2, 2023
@Gelio
Copy link
Contributor Author

Gelio commented Jan 2, 2023

I don't mind not having this feature in immer. I am wondering what do you mean by saying that this feature never landed. What feature never landed?

@mweststrate
Copy link
Collaborator

This PR :)

@Gelio
Copy link
Contributor Author

Gelio commented Jan 2, 2023

Oh. It is disappointing to see a PR closed because it was not merged without any more justification. Especially that there was no delay and no asks from me at any point. I feel like my time was wasted. #696 is not closed and still encourages implementing this idea.

@mweststrate
Copy link
Collaborator

mweststrate commented Jan 2, 2023

Yeah sorry about that! Currently Immer has 5 million dependent packages on Github, and the idea has 3 upvotes, so it seems hard to justify at this moment to ship the additional code and increased maintenance complexity to everyone. At this moment it'd become a classic example of feature creep I think :)

@Gelio
Copy link
Contributor Author

Gelio commented Jan 2, 2023

Sure, makes sense, thanks for a more thorough explanation 馃憤

@Gelio Gelio deleted the support-typed-arrays branch January 2, 2023 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typed Arrays support
2 participants