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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

maybeDeepFreeze takes ~200ms on big arrays #7474

Closed
Banou26 opened this issue Dec 13, 2020 · 3 comments
Closed

maybeDeepFreeze takes ~200ms on big arrays #7474

Banou26 opened this issue Dec 13, 2020 · 3 comments

Comments

@Banou26
Copy link
Contributor

Banou26 commented Dec 13, 2020

Intended outcome:
Apollo not taking ~200ms deepFreezing big arrays when I'm updating a reactive variable.
Or maybe make an option for us to disable deepFreeze in development as it'd also fix #6813.

Actual outcome:
Apollo deepFreeze the array everytime i make a change to the reactive variable and it takes around 200ms.

How to reproduce the issue:
Set a big array (more than 1k items) of objects in a reactive array & make a query that fetches it.

Versions
"@apollo/client": "^3.2.9"

@benjamn
Copy link
Member

benjamn commented Dec 14, 2020

@Banou26 Very much appreciate your performance stress-testing!

I'm cautiously optimistic that this PR could have a big impact on the amount of freezing that happens in development. Specifically, since result objects are canonicalized before they're frozen in the new system, they won't be re-frozen if the canonical object ends up having the same structure as it had before (even though it might not be === to the previous object, prior to canonicalization). I'm guessing that most of your array elements are unchanged in structure, so they wouldn't need to be repeatedly re-frozen. Also, maybeDeepFreeze will no longer be used for freezing result objects, since we can incrementally freeze new objects using a shallow Object.freeze call. Finally, in this system, only [object Array] and [object Object] objects get frozen, which should finally fix #6813, since byte arrays have Object.prototype.toString.call(array) === "[object Uint8Array]".

@jimrandomh
Copy link

Also note: if you aren't using a bundler that replaces process.env.NODE_ENV at compile time (eg if you're using Meteor, or if you're not doing bundling of server code at all), you'll get bad performance, and maybeDeepFreeze is one of the places that's heavily impacted. See this PR.

@hwillson
Copy link
Member

It doesn't sound like there is an outstanding issue here, so closing. Thanks!

@hwillson hwillson removed this from the MM-2021-06 milestone Jul 29, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants