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

Faster iteration experiment #1120

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Conversation

mweststrate
Copy link
Collaborator

@mweststrate mweststrate commented Apr 27, 2024

Object.keys is faster than Reflect.keys. Can we leverage this somehow? (at the cost of symbol props etc) https://www.measurethat.net/Benchmarks/Show/18717/0/reflectownkeys-vs-objectkeys-preformence#latest_results_block

@markerikson
Copy link
Contributor

FWIW, I've actually been doing some RTK perf optimizations over in reduxjs/redux-toolkit#4361 (comment) , and found that 1) it was faster to do some reads on a const ids = current(state.ids) array instead of a wrapped array, 2) the majority of the remaining overhead seems to be in Immer, 3) a lot of it looks like it's in finalizeProperties

@mweststrate
Copy link
Collaborator Author

See also #1107. There are several ways to make Immer quite a bit faster, but it would required much more loosely treating things that have getters/ non-enumerables / symbolic properties, which would benefit the majority of the user base, but also create enough failure scenarios to flood the issue tracker😅

@markerikson
Copy link
Contributor

@mweststrate out of curiosity, how much of a perf difference do you think this will make?

FWIW it looks like only one test failed:

FAIL __tests__/base.js
  ● Test suite failed to run

    TypeError: Cannot perform 'get' on a proxy that has been revoked

      2379 | 		})
      2380 |
    > 2381 | 		expect(state[sym].x).toBe(2)
           | 		                  ^
      2382 | 	})
      2383 | }
      2384 |

      at __tests__/base.js:2381:21
      at testObjectTypes (__tests__/base.js:2368:2)
      at __tests__/base.js:1861:28
      at __tests__/base.js:1860:3
      at runBaseTest (__tests__/base.js:61:2)
      at Object.<anonymous> (__tests__/base.js:32:4)

@mweststrate
Copy link
Collaborator Author

@Pyaepyae-art please stop randomly approving draft or merged PRs. I'm not sure why you are doing this. I hope it is unintentional. But just in case, on the next contextless review you will be banned from the repo.

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.

2 participants