-
-
Notifications
You must be signed in to change notification settings - Fork 38
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
Optimization of the optimization #39
Comments
Hi @olee, |
You are right and wrong a the same time 😄 I checked it in detail once more and noticed you actually only generate "thumbprints" of the passed object for plain objects where the values in the object are only primitives which was a really good consideration and would indeed ensure that the function cannot crash afaik. However, for ensuring performance in all cases and to be in line with React's rules of hooks, I still think the option I proposed would be more beneficial.
Trying to use the option I proposed with using useMemo would be even be destroyed by your code in some cases: // This code would work with the current memoization, but even though the very same object is passed, the json serialization and all the other checks would still run
const { classes } = useStyles(
useMemo(() => ({
color: props.color,
}), [props.color])
);
// This code would always recompute classes, even if the object is the same
const { classes } = useStyles(
useMemo(() => ({
color: props.color,
componetOptions: {
bold: props.bold,
italic: false,
}
}), [props.color, props.bold])
); This is why I think it is a better option to stay with "simple is best" here and leave the details to the developer actually implementing the styles 👍 |
It can. Getters can throw exceptions.
Big no, I will never recommend implementing this approach. It obfuscates the code and requires additional maintenance for the sake of a very hypothetical performance gain.
There is no JSON serialization going on. I used
No, unpredictable performances.
Passing the
No, it won't.
In the early days of
Now, all that said, it is very much possible that the optimization currently in place is detrimental for the performances. I didn't do any complexity analysis nor benchmark. Thank you for your input. I'll close this now but you are more than welcome to help me find a solution for |
In this line,
getDependencyArrayRef(params)
should be replaced with justparams
.First reason:
getDependencyArrayRef
uses json serialization which can VERY easily throw exceptions if it is not a plain object or contains circular referencesSecond reason:
getDependencyArrayRef
uses json serialization which has a huge negative performance impact again which counteracts the optimizationThird reason:
It's not even required. If the object is the same reference, we should also expect that it's values are the same (same as with React's own hooks like
useEffect
oruseMemo
).This would of course break if someone actually does something crazy like this:
However, as mentioned before, this is the very same for React's own hooks as well so it does not make sense to try optimizing for this.
Fourth reason:
If a user does want to use params but also benefit from performance improvements, there is a simpler and way more performant way to do so:
The text was updated successfully, but these errors were encountered: