-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Avoid unnecessary selector evaluations #1273
Avoid unnecessary selector evaluations #1273
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR makes no sense in my opinion (and it bleeds changes from #1272), see my inline comment
1de42aa
to
5ee5de3
Compare
5ee5de3
to
32d81d5
Compare
I will close this for now, sorry 😅 |
32d81d5
to
71dfe44
Compare
I'm reopening this PR. I think that this should actually make a difference in terms of performance, because the selector will only be evaluated when it's strictly necessary. I guess that it would only make sense to merge this PR if the performance benchmarks are actually better, otherwise it wouldn't make sense to merge it, of course. Please @MrWolfZ , could you please have another look at this 🙏? Thanks a lot! |
c332fa3
to
5d276ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this change doesn't really add anything and makes the code just more complex. If the user cares about the selector logic being called too often, let them use a memoizing selector.
Also, since this change is supposed to bring better performance, please run this code against the benchmark repository.
I honestly don't think that it makes the code much more complex than what it already was, but that is subjective, of course.
But this is the thing, even if the user memoized the selector, the selector would be executed twice when an update is detected and when the render that happens after that takes place... Memoized selectors are cheap, but they are not free, in the case of reselect the whole tree of selectors has be be re-evaluated with their latest arguments, if we can avoid that I think that we should. Not to mention the fact that lots of ppl don't memoize their selectors. Also, without this possible improvement, any time that there is an update from an ancestor, if that update is not changing the selector, the selector would be evaluated again... Which is totally unnecessary. Honestly, I don't understand why you are so against this.
I would love to do that. |
Also @MrWolfZ, please look at the bright side. Even if this change is actually bad or unnecessary, at the very least it will help us to come up with more and better tests for possible future refactors. So far we already got one new test 😉 |
10cf5f4
to
88dce64
Compare
The latest version of this code looks much better, and I think it is now correct. However, I am still doubtful whether it's worth it. Here's why:
But again, the code seems correct, and as demonstrated it can provide performance improvements. I can absolutely see both sides of the argument. Since I am not the maintainer of this repo, I can't decide this in any case. It is up to the maintainers to decide whether they prefer simpler more maintainable code or more performant code. |
a208a7b
to
d129ef1
Compare
EditIn here there was an answer to @MrWolfZ comment that now I wish I had handled better. That's why I'm making this final edit. If you are curious to see that comment, then use the "edit" dropdown. 🙂 |
95abfb4
to
28f928a
Compare
Kind of an obvious question: Why would the selector ever change? I'm pretty sure Dan's hand will emerge from your computer monitor and slap you in the face if you ever did this: useEffect(user_id ? loadUserEffect : loadLoginEffect) While not enforced by a eslint rule, that kind of thing is basically the same as putting your Hooks inside a conditional. It's objectively bad code and should feel bad. I think that kind of makes this change a moot point, no? |
For instance: function User({ id }) {
const getUser = useCallback(state => state.users[id], [id]);
const user = useSelector(getUser);
// return some elements to show the user-data here
} It's quite likely that the id will also be the key of the Component, in which case the id would never change, of course... But check this out: function UsersDiff({ idA, idB }) {
const getUserA = useCallback(state => state.users[idA], [idA]);
const getUserB = useCallback(state => state.users[idB], [idB]);
const userA = useSelector(getUserA);
const userB = useSelector(getUserB);
// return some elements that show the differences or whatever
} In the latest case, the IDs could change and the key of the component wouldn't be based on those IDs. The only reason why the selector could change is that this selector -unlike the selector of
I'm having a hard time picturing Dan slapping someone in the face, but I guess that I get what you meant 😄 |
8d626fd
to
5ab306b
Compare
Hi @MrWolfZ @timdorr and @markerikson, Sorry that it took me a while to post the results. I got started at a new job and I have been quite busy for the last 2 weeks. These results are obtained with the code of this branch. You may want to have a look at it just to make sure that I didn't screw anything. If I'm reading these results correctly, they are showing an improvement on performance and that is despite the fact that the selectors of the benchmarks don't actually perform any heavy computations. These are the results: Running benchmark deeptree
react-redux version: 7.0.0.alpha-avoidSelectorEvaluation
Checking max FPS... (30 seconds)
Running trace... (30 seconds)
react-redux version: 7.0.0.alpha-useSelectorNoDeps
Checking max FPS... (30 seconds)
Running trace... (30 seconds)
Results for benchmark deeptree:
┌─────────────────────────────────────┬─────────┬──────────────┬───────────┬───────────┬──────────┬─────────────────────────────────────────────────────────────────────────┐
│ Version │ Avg FPS │ Render │ Scripting │ Rendering │ Painting │ FPS Values │
│ │ │ (Mount, Avg) │ │ │ │ │
├─────────────────────────────────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼─────────────────────────────────────────────────────────────────────────┤
│ 7.0.0.alpha-avoidSelectorEvaluation │ 44.06 │ 86.9, 0.6 │ 9452.44 │ 10396.98 │ 6063.76 │ 56,57,58,55,57,58,57,58,41,35,37,34,35,34,33,24,27,24,24 │
├─────────────────────────────────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼─────────────────────────────────────────────────────────────────────────┤
│ 7.0.0.alpha-useSelectorNoDeps │ 42.27 │ 88.8, 0.6 │ 9521.25 │ 10229.33 │ 6096.60 │ 56,55,56,55,56,53,54,53,56,52,53,37,33,37,33,34,33,32,24,21,23,22,25,25 │
└─────────────────────────────────────┴─────────┴──────────────┴───────────┴───────────┴──────────┴─────────────────────────────────────────────────────────────────────────┘
Running benchmark deeptree-nested
react-redux version: 7.0.0.alpha-avoidSelectorEvaluation
Checking max FPS... (30 seconds)
Running trace... (30 seconds)
react-redux version: 7.0.0.alpha-useSelectorNoDeps
Checking max FPS... (30 seconds)
Running trace... (30 seconds)
Results for benchmark deeptree-nested:
┌─────────────────────────────────────┬─────────┬──────────────┬───────────┬───────────┬──────────┬────────────────────────────────────────────────────────────────────────────┐
│ Version │ Avg FPS │ Render │ Scripting │ Rendering │ Painting │ FPS Values │
│ │ │ (Mount, Avg) │ │ │ │ │
├─────────────────────────────────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼────────────────────────────────────────────────────────────────────────────┤
│ 7.0.0.alpha-avoidSelectorEvaluation │ 56.75 │ 128.6, 0.3 │ 3937.45 │ 2915.23 │ 1803.65 │ 58,57,60,59,60,59,56,58,59,58,60,59,58,57,60,57,55,54,53,55,52,51,49,53,53 │
├─────────────────────────────────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼────────────────────────────────────────────────────────────────────────────┤
│ 7.0.0.alpha-useSelectorNoDeps │ 56.87 │ 126.1, 0.4 │ 3993.32 │ 2951.71 │ 1872.79 │ 58,60,59,60,58,57,58,60,59,58,59,58,56,59,56,53,55,53,56,55,51,54,54 │
└─────────────────────────────────────┴─────────┴──────────────┴───────────┴───────────┴──────────┴────────────────────────────────────────────────────────────────────────────┘
Running benchmark forms
react-redux version: 7.0.0.alpha-avoidSelectorEvaluation
Checking max FPS... (30 seconds)
Running trace... (30 seconds)
react-redux version: 7.0.0.alpha-useSelectorNoDeps
Checking max FPS... (30 seconds)
Running trace... (30 seconds)
Results for benchmark forms:
┌─────────────────────────────────────┬─────────┬──────────────┬───────────┬───────────┬──────────┬─────────────────────────────────────────────────────────────────────────┐
│ Version │ Avg FPS │ Render │ Scripting │ Rendering │ Painting │ FPS Values │
│ │ │ (Mount, Avg) │ │ │ │ │
├─────────────────────────────────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼─────────────────────────────────────────────────────────────────────────┤
│ 7.0.0.alpha-avoidSelectorEvaluation │ 56.26 │ 943.0, 0.2 │ 6138.50 │ 972.85 │ 2727.64 │ 55,57,56,55,57,55,57,56,57,60,56,57,56,53,58,56,57,55,57,55,56,55,55 │
├─────────────────────────────────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼─────────────────────────────────────────────────────────────────────────┤
│ 7.0.0.alpha-useSelectorNoDeps │ 55.58 │ 898.3, 0.3 │ 6272.87 │ 1015.13 │ 2758.28 │ 56,57,56,55,56,55,57,56,57,56,57,56,57,52,58,57,53,56,54,56,55,50,55,55 │
└─────────────────────────────────────┴─────────┴──────────────┴───────────┴───────────┴──────────┴─────────────────────────────────────────────────────────────────────────┘
Running benchmark stockticker
react-redux version: 7.0.0.alpha-avoidSelectorEvaluation
Checking max FPS... (30 seconds)
Running trace... (30 seconds)
react-redux version: 7.0.0.alpha-useSelectorNoDeps
Checking max FPS... (30 seconds)
Running trace... (30 seconds)
Results for benchmark stockticker:
┌─────────────────────────────────────┬─────────┬──────────────┬───────────┬───────────┬──────────┬────────────────────────────────────────────────────────────────────────────┐
│ Version │ Avg FPS │ Render │ Scripting │ Rendering │ Painting │ FPS Values │
│ │ │ (Mount, Avg) │ │ │ │ │
├─────────────────────────────────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼────────────────────────────────────────────────────────────────────────────┤
│ 7.0.0.alpha-avoidSelectorEvaluation │ 38.37 │ 170.5, 3.3 │ 18877.63 │ 7037.41 │ 2102.51 │ 40,39,41,38,39,40,38,39,41,38,39,38,39,40,38,37,36,38,37,36,36 │
├─────────────────────────────────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼────────────────────────────────────────────────────────────────────────────┤
│ 7.0.0.alpha-useSelectorNoDeps │ 37.07 │ 168.9, 3.3 │ 19129.98 │ 6931.06 │ 2035.21 │ 39,40,42,39,37,38,37,36,39,35,36,37,35,34,37,38,39,37,35,36,37,34,36,37,37 │
└─────────────────────────────────────┴─────────┴──────────────┴───────────┴───────────┴──────────┴────────────────────────────────────────────────────────────────────────────┘
Running benchmark tree-view
react-redux version: 7.0.0.alpha-avoidSelectorEvaluation
Checking max FPS... (30 seconds)
Running trace... (30 seconds)
react-redux version: 7.0.0.alpha-useSelectorNoDeps
Checking max FPS... (30 seconds)
Running trace... (30 seconds)
Results for benchmark tree-view:
┌─────────────────────────────────────┬─────────┬──────────────┬───────────┬───────────┬──────────┬───────────────────────────────────────────────────────────────────────────────┐
│ Version │ Avg FPS │ Render │ Scripting │ Rendering │ Painting │ FPS Values │
│ │ │ (Mount, Avg) │ │ │ │ │
├─────────────────────────────────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼───────────────────────────────────────────────────────────────────────────────┤
│ 7.0.0.alpha-avoidSelectorEvaluation │ 50.04 │ 426.4, 0.9 │ 5597.07 │ 12955.30 │ 463.26 │ 55,39,53,55,42,48,53,52,54,41,52,46,49,50,54,51,55,52,54,52,48,51,48,51,50,50 │
├─────────────────────────────────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼───────────────────────────────────────────────────────────────────────────────┤
│ 7.0.0.alpha-useSelectorNoDeps │ 48.20 │ 422.7, 1.1 │ 6539.58 │ 9667.43 │ 544.14 │ 42,49,53,44,49,51,55,52,43,50,47,44,46,49,52,47,53,44,49,43,55,50,51,44,53,53 │
└─────────────────────────────────────┴─────────┴──────────────┴───────────┴───────────┴──────────┴───────────────────────────────────────────────────────────────────────────────┘
Running benchmark twitter-lite
react-redux version: 7.0.0.alpha-avoidSelectorEvaluation
Checking max FPS... (30 seconds)
Running trace... (30 seconds)
react-redux version: 7.0.0.alpha-useSelectorNoDeps
Checking max FPS... (30 seconds)
Running trace... (30 seconds)
Results for benchmark twitter-lite:
┌─────────────────────────────────────┬─────────┬──────────────┬───────────┬───────────┬──────────┬──────────────────────────────────────────────────────────────────────────────────┐
│ Version │ Avg FPS │ Render │ Scripting │ Rendering │ Painting │ FPS Values │
│ │ │ (Mount, Avg) │ │ │ │ │
├─────────────────────────────────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼──────────────────────────────────────────────────────────────────────────────────┤
│ 7.0.0.alpha-avoidSelectorEvaluation │ 47.54 │ 2.2, 0.5 │ 21001.58 │ 3599.72 │ 382.40 │ 59,60,59,60,58,59,60,58,56,50,47,44,40,39,34,29,28,27,27 │
├─────────────────────────────────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼──────────────────────────────────────────────────────────────────────────────────┤
│ 7.0.0.alpha-useSelectorNoDeps │ 47.39 │ 2.1, 0.5 │ 21157.33 │ 3660.14 │ 403.49 │ 59,60,58,59,58,59,56,59,58,59,55,57,55,54,48,46,44,41,38,34,33,32,31,30,29,28,28 │
└─────────────────────────────────────┴─────────┴──────────────┴───────────┴───────────┴──────────┴──────────────────────────────────────────────────────────────────────────────────┘
Done in 862.88s. I have also tried benchmarking the same, but making the selectors perform some silly work on purpose, and as I already thought the results in those cases show a much more significant improvement in performance in favor of the code of this PR. However, I think that just with these results we should consider merging this PR 🙂 |
FWIW, FPS values will fluctuate from run to run. Anything +-3-ish FPS should be basically considered equal. |
Sure, this optimization is only meant to be significant for those selectors that perform computations. All the selectors of our benchmarks are trivial selectors that only select things directly from the state... What I meant by showing these results is that this change can only make things better, never worse. Do you want me to share the results and the code that test how this change would perform on non-trivial selectors? Do you want me to create a new benchmark that has some non-trivial selectors? Do you want me to just close this PR given those results? Thanks! |
I haven't made up my mind on this one overall. Still trying to get a better understanding of the tradeoffs and weighing options. So yes, more benchmarks, examples, and edge cases would be helpful. |
@josepot : hmm. apologies, I'm getting the PRs mixed up :) Gimme some time to sit down and actually evaluate this one in the next few days. |
Awright. I don't see any harm in this. I did swap the ternary for an if/else, though. Not a ternary fan. |
* Avoid unnecessary selector evaluations * Clean up state assignment logic * Add missing shallowEqual export
* Avoid unnecessary selector evaluations * Clean up state assignment logic * Add missing shallowEqual export
* Avoid unnecessary selector evaluations * Clean up state assignment logic * Add missing shallowEqual export
Hi,
While I was working on #1272 I noticed that the selector was being evaluated more times than what's necessary. This PR makes sure that the selector is only evaluated when it's strictly needed.
cc: @MrWolfZ I would really appreciate you having a look at this, just to make sure that I didn't miss anything. All the previous tests are passing, and I have also added a new one to make sure that updating the selector works correctly. Please let me know what you think. Thanks!
Edit
A couple of things that I forgot to mention that perhaps aren't that obvious:
The only commit that's relevant for this PR is this one. The other one is the commit of Remove deps of useSelector #1272... I'm just being a bit optimistic and hoping that Remove deps of useSelector #1272 will get merged before this one.
My rationale for this change is the following:
If the update comes as a result of the
forceRender
ofcheckForUpdates
, then there is no need to re-evaluate the selector.If the update comes from "the outside", then it could be that:
checkForUpdates
will already pick that change and trigger a render... Chances are that the child-update will have happened before the parent one, but it doesn't really matter, because it will happen during the same loop, so it will be batched... 🙂