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

Immer 7 is 8 times slower than immer 6 #681

Closed
1 task
lpatiny opened this issue Oct 8, 2020 · 13 comments
Closed
1 task

Immer 7 is 8 times slower than immer 6 #681

lpatiny opened this issue Oct 8, 2020 · 13 comments

Comments

@lpatiny
Copy link

@lpatiny lpatiny commented Oct 8, 2020

馃悰 Bug Report

We have a (too) complex open source project that allows to display scientific data https://github.com/cheminfo/nmr-displayer

Going from immer 6.0.9 to immer 7.0.9 slow down the vertical scale of spectra (mouse wheel) by a factor 8 !

Link to reproduce

Here are 2 links:

Version build with immer 7.0.9:

https://www.nmrium.org/v0.3.3/#/SamplesDashboard/afp2vszl3kj/cytisine2dhttps:wwwnmriumorgdatacytisine2dhsqcjson?sampleURL=https%3A//www.nmrium.org/samples.json#/

Version build with immer 6.0.9

https://www.nmrium.org/v0.3.5/#/SamplesDashboard/ul8f8s4969/undefinedcytisine2dhttps:wwwnmriumorgdatacytisine2dhmbc-onlyjson?sampleURL=https%3A//www.nmrium.org/samples.json#/

In attachments videos displaying the difference of speed Archive.zip

To Reproduce

Click on the link and you may zoom on this area

image

Then you may use the scroll button on the mouse to vertically scale the spectrum. You will see that version 6 is 8 times faster than version 7. This can be confirmed in the google chrome performance tab.

Observed behavior

Version 7 is 8 times slower than version 6

Expected behavior

speed should be the same in version 6 and 7

Environment

Google chrome

  • Immer version:
  • I filed this report against the latest version of Immer
@mweststrate
Copy link
Collaborator

@mweststrate mweststrate commented Oct 8, 2020

@lpatiny
Copy link
Author

@lpatiny lpatiny commented Oct 8, 2020

I measured the time using google chrome performance to vertical scale one level

autoFreeze(false);: ~1600 ms

image

autoFreeze(true): ~220 ms

image

(there is a branch for testing: https://github.com/cheminfo/nmr-displayer/tree/immer7)

@mweststrate
Copy link
Collaborator

@mweststrate mweststrate commented Oct 8, 2020

@lpatiny
Copy link
Author

@lpatiny lpatiny commented Oct 8, 2020

autoFreeze(true) gives the expected performance, same performance that in the developement mode

When we have autoFreeze(false) we are 8 times slower, the same as with the build application

@mweststrate
Copy link
Collaborator

@mweststrate mweststrate commented Oct 8, 2020

@lpatiny
Copy link
Author

@lpatiny lpatiny commented Oct 8, 2020

Great many thanks. I will implement the autoFreeze(true) in the meantime

@lpatiny lpatiny closed this as completed Oct 8, 2020
@mweststrate
Copy link
Collaborator

@mweststrate mweststrate commented Oct 8, 2020

Reopening temporarily to be able to refer to it in the changelog

@mweststrate mweststrate reopened this Oct 8, 2020
@ctrlplusb
Copy link

@ctrlplusb ctrlplusb commented Oct 13, 2020

This seems like a big deal, a complete u-turn on previous recommendations / understanding around the performance characteristics of Immer. 馃槷

Could we consider adding the ability to scope the freeze flag, enabling or disabling it for a particular produce call, rather than toggling it on/off at a global level? At least then each package can make its own decisions around how it wants to optimize around the freezing feature without triggering a global flag that may break the expectations for other modules.


Update

Ok, dug into source and noted that the Immer class is being exported, allowing this at an instance level at least.

import { Immer } from 'immer';

const neverAutoFreezeImmer = new Immer({
  useProxies: typeof window !== 'undefined' && typeof window.Proxy !== 'undefined',
  autoFreeze: false,
});

mweststrate added a commit that referenced this issue Nov 17, 2020
BREAKING CHANGE: always freeze by default, even in production mode. Use `setAutoFreeze(process.env.NODE_ENV !== 'production')` for the old behavior. See #687 (comment) for the rationale. Fixes #649, #681, #687
@mweststrate
Copy link
Collaborator

@mweststrate mweststrate commented Nov 19, 2020

Fixed in 8.0.0.

@ctrlplusb note that the u turn is mostly caused by an earlier major release (7 iirc) that changed the performance characteristics, so the new default brings it better in line with how Immer used to behave before.

@ashot
Copy link

@ashot ashot commented Mar 3, 2021

This doesn't seem like a great fix because it means you can never mutate data outside of a producer for data managed by immer, without taking a huge performance hit (unless I'm missing something).

For some cases its useful to have that escape valve (see "you can always opt out" https://immerjs.github.io/immer/docs/performance#you-can-always-opt-out).

Is having an option to turn off pruning potentially a solution here?

@mweststrate
Copy link
Collaborator

@mweststrate mweststrate commented Mar 3, 2021

@ashot
Copy link

@ashot ashot commented Mar 3, 2021

But how can you write the logic by hand if the object is frozen?

@mweststrate
Copy link
Collaborator

@mweststrate mweststrate commented Mar 3, 2021

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

No branches or pull requests

4 participants