Skip to content

Investigate optimizations for immutable invariant middleware #926

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

Closed
markerikson opened this issue Mar 15, 2021 · 5 comments · Fixed by #943
Closed

Investigate optimizations for immutable invariant middleware #926

markerikson opened this issue Mar 15, 2021 · 5 comments · Fixed by #943

Comments

@markerikson
Copy link
Collaborator

markerikson commented Mar 15, 2021

The immutable invariant middleware is relatively slow, because it does deep checks on the whole state, and uses recursion.

I wonder if we'd be able to speed it up by:

  • Converting the recursion to some sort of a loop with a queue
  • Skipping certain items if they're already frozen

Granted, this only affects dev mode and not prod, but if we can speed it up a bit that would be a net win.

There are some other mutation detection options out there that involve proxies, like https://www.npmjs.com/package/mutation-sentinel and https://www.npmjs.com/package/will-mutate , but not sure if those approaches would work sufficiently for our use case.

@markerikson
Copy link
Collaborator Author

So last night I kinda hacked together an alternate implementation of the track and detectMutations functions based on a queue.

It also seems, at first glance, to be 10x slower than the existing implementation :) I'll have to play around with it further and figure out why.

@markerikson
Copy link
Collaborator Author

For posterity: the queue approach I wrote just turned out to be drastically slower. Weirdly, after doing some performance profiling, the biggest hotspots appeared to be, uh... inserting a new field into a cache object and reading it out again later? Still no idea how that's so slow, unless it had to do with resizing the internal representation of the object to hold around 90000 keys.

That said, the huge win for optimizing the original algorithm was treating frozen values as immutable, which lets us almost always bail out after just skimming past the top level values of the root state. Additionally, switching from keypath arrays (with lots of .concat() and .join() calls) to just pre-calculated dotted strings was another big perf win.

@codenickycode
Copy link

codenickycode commented May 11, 2021

Hi Mark, Sorry I'm late posting this as a stress test related to this thread. Hope it helps if you still need it:

https://sequencer64.com
https://github.com/drumnickydrum/sequencer64

(You requested in https://stackoverflow.com/questions/66781880/redux-toolkit-thunk-disappears-after-getdefaultmiddlware-config )

@markerikson
Copy link
Collaborator Author

@drumnickydrum oh hey, I saw you post this to HN earlier today - looks really neat!

The RTK 1.5.1 release should have drastically sped up the immutability checks - we assume things are fine if objects are frozen and don't recurse further, and also optimized some string usages.

It looks like your project is in fact on RTK 1.5.1. Could you do me a favor and try turning those dev checks back on to see how it performs in dev? My guess is you should see almost no noticeable slowdown with those enabled.

Also, would love to hear any thoughts you have in general on using RTK overall, and especially in a project like this.

@codenickycode
Copy link

codenickycode commented May 13, 2021 via email

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 a pull request may close this issue.

2 participants