Skip to content

Commit

Permalink
Faster shouldUpdate (#6070)
Browse files Browse the repository at this point in the history
**Problem:**
See #6066

**Fix:**
The major contributor to the slowness of `isElementInChildrenOrPropsTree
` is `React.children.toArray`.
I tried to use `React.children.forEach` or `React.children.map` but
those just made things worse.
However, we know that in lot of cases `props.children` is just an array
of elements or a React element itself. In these cases there is no need
to run the fully general and slow toArray. Even though this is not the
official use of the API (it is actually discouraged:
https://react.dev/reference/react/Children#why-is-the-children-prop-not-always-an-array),
it makes sense to try to get the elements without calling toArray (and
we can still call `toArray` when that fails).
I made the function even more faster by switching to simple `for` loops
instead of using array functions and allocating even more arrays.
With these changes I never saw >1.3ms shouldUpdate times when resizing a
PromiseCard.

**Manual Tests:**
I hereby swear that:

- [x] I opened a hydrogen project and it loaded
- [x] I could navigate to various routes in Preview mode

Fixes #6066
  • Loading branch information
gbalint authored Jul 12, 2024
1 parent 67abcdd commit ec55489
Showing 1 changed file with 32 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -380,20 +380,40 @@ function isRenderProp(prop: any): prop is { props: { [UTOPIA_PATH_KEY]: string }
}

function isElementInChildrenOrPropsTree(elementPath: string, props: any): boolean {
const childrenArr = React.Children.toArray(props.children).filter(React.isValidElement)
const elementIsChild = childrenArr.some((c) => (c.props as any)[UTOPIA_PATH_KEY] === elementPath)
if (elementIsChild) {
return true
const childrenArr = fastReactChildrenToArray(props.children)
for (let c of childrenArr) {
if ((c.props as any)[UTOPIA_PATH_KEY] === elementPath) {
return true
}
}

const elementsInProps = Object.values(props).filter(isRenderProp)
const isElementInProps = elementsInProps.some((p) => p.props[UTOPIA_PATH_KEY] === elementPath)
if (isElementInProps) {
return true
for (let p of props) {
if (React.isValidElement(p) && (p.props as any)[UTOPIA_PATH_KEY] === elementPath) {
return true
}
}

return (
childrenArr.some((c) => isElementInChildrenOrPropsTree(elementPath, c.props)) ||
elementsInProps.some((p) => isElementInChildrenOrPropsTree(elementPath, p.props))
)
for (let c of childrenArr) {
if (isElementInChildrenOrPropsTree(elementPath, c.props)) {
return true
}
}

for (let p in props) {
if (isRenderProp(p) && isElementInChildrenOrPropsTree(elementPath, p.props)) {
return true
}
}

return false
}

function fastReactChildrenToArray(children: any) {
if (React.isValidElement(children)) {
return [children]
}
if (Array.isArray(children)) {
return children.filter(React.isValidElement)
}
return React.Children.toArray(children).filter(React.isValidElement)
}

0 comments on commit ec55489

Please sign in to comment.