Avoid work inside of render() #50
Conversation
Our HOC here is currently doing more work than is really necessary inside of render. This can make updates slower than they need to be. To optimize this some, I'm moving this work into the constructor and componentWillReceiveProps to avoid unnecessarily repeating it.
lencioni
force-pushed
the
reduce-render-work
branch
from
f4fabda
to
b62bdc3
Compare
| @@ -56,15 +75,7 @@ export function withStyles( | |||
| ThemedStyleSheet.flush(); | |||
| } | |||
|
|
|||
| const addedProps = { | |||
| [themePropName]: ThemedStyleSheet.get(themeName), | |||
There was a problem hiding this comment.
Is ThemedStylesheet.get guaranteed to be idempotent based on the themeName?
If not, you may need to call it unconditionally in cWRP.
There was a problem hiding this comment.
yes, it's guaranteed. get(x) === get(x). always. Similarly, styleDef(x) === styleDef(x) always, and only does a meaningful amount of work (ie, more than a hash lookup) on the first call. As a result, i'm not really sure that this PR will actually have a meaningful impact on performance, and could even be negative, due to the extra lifecycle events. Have we benchmarked any of this?
There was a problem hiding this comment.
I haven't benchmarked yet. I plan to do that on Monday though!
There was a problem hiding this comment.
Code LGTM, pending benchmarks per @lelandrichardson's comment
|
Performance seems basically identical before and after this change, at least in Chrome, on a dev build of React. |
|
Yeah, I wouldn't imagine this changing any of the performance characteristics. good to verify though! If you're looking for ways to eek out performance for the style pipeline in react, we should chat. I have a couple of ideas that are react-primitives specific, but might end up applying here. |
Our HOC here is currently doing more work than is really necessary
inside of render. This can make updates slower than they need to be. To
optimize this some, I'm moving this work into the constructor and
componentWillReceiveProps to avoid unnecessarily repeating it.
@airbnb/webinfra