refactor: avoid calling traverseAndSetElement on every rerender #3998
Conversation
| if (currParts.length !== prevParts?.length) { | ||
| // It should not be possible to reach this point as patching only occurs when the vnode is the same. | ||
| if (process.env.NODE_ENV !== 'production') { | ||
| throw new Error('Expected static parts to be the same for the same element'); | ||
| } | ||
| // Recover from patching issue | ||
| return applyStaticParts(root, n2, renderer, false); | ||
| } |
There was a problem hiding this comment.
This should technically not be reachable because patch should only occur if a new vnode is generated that's the same as the previous one.
If the vnodes are reused we skip the patching process.
I'm adding this part more as a cautionary check in case something goes wrong and we have a way to recover from it.
I thought about just throwing an error here but it doesn't seem helpful as this would more likely be an issue with the engine and not something the component author could resolve.
There was a problem hiding this comment.
I understand the desire for the safeguard, but if we just throw Error without calling applyStaticParts (similar to how we handle it in static-parts.ts), then applyStaticParts will only ever be called with mount = true, and we can just rename it to mountStaticParts and simplify the code.
I think we can be reasonably certain that this code won't ever be hit, so I would say YAGNI and go with the simpler version for now. If not, we know how to fix in patch. 🙂
There was a problem hiding this comment.
@nolanlawson I thought about it and I think you're right we probably won't hit this safeguard.
If we do run into this issue, it would be an issue with patching in general and more specifically why we are attempting to patch two different vnodes.
The error handling should be done higher in the call stack in that scenario.
I updated in the latest commit!
| for (const [i, part] of currParts.entries()) { | ||
| // Patch only occurs if the vnode is newly generated, which means the part.elm is always undefined | ||
| part.elm = prevParts[i].elm; | ||
| // Refs must be updated after every render due to refVNodes getting reset before every render | ||
| applyRefs(part, currPartsOwner); | ||
| } |
There was a problem hiding this comment.
We already have the cached static parts on the previous vnode so we can just reassign the values from the previous vnode to the current one.
I thought about keeping the list of static parts to a separate map or data structure but since we need to iterate through them to reapply the refs anyways, it seemed unneccesary.
Additionally, we may need to iterate through the parts for the remaining work in #3624, ie updating dynamic attributes, styles, classes, etc.
Leaving it as is for now until the remaining work is completed and then plan to revisit this.
In a future optimization, we may be able to keep track of the parts that have changed and only update those ones instead of iterating through the entire list of parts.
| * @param renderer - the renderer to use | ||
| */ | ||
| export function patchStaticParts(n1: VStatic, n2: VStatic, root: Element, renderer: RendererAPI) { | ||
| if (!process.env.IS_BROWSER) { |
There was a problem hiding this comment.
Could you remind me of the reason for this? Because we don't do any patching at all on the server?
There was a problem hiding this comment.
I think it's because in @lwc/engine-server we don't support refs or event listeners but TBH I'm using it because it was in applyStaticParts.
This might change when we're looking at the attributes/styles/classes though. I'll repeat the comment from applyStaticParts so we can keep track of the reasoning.
packages/@lwc/engine-core/src/framework/modules/static-parts.ts
Outdated
Show resolved
Hide resolved
| if (currParts.length !== prevParts?.length) { | ||
| // It should not be possible to reach this point as patching only occurs when the vnode is the same. | ||
| if (process.env.NODE_ENV !== 'production') { | ||
| throw new Error('Expected static parts to be the same for the same element'); | ||
| } | ||
| // Recover from patching issue | ||
| return applyStaticParts(root, n2, renderer, false); | ||
| } |
There was a problem hiding this comment.
I understand the desire for the safeguard, but if we just throw Error without calling applyStaticParts (similar to how we handle it in static-parts.ts), then applyStaticParts will only ever be called with mount = true, and we can just rename it to mountStaticParts and simplify the code.
I think we can be reasonably certain that this code won't ever be hit, so I would say YAGNI and go with the simpler version for now. If not, we know how to fix in patch. 🙂
| return applyStaticParts(root, n2, renderer, false); | ||
| } | ||
|
|
||
| for (const [i, part] of currParts.entries()) { |
There was a problem hiding this comment.
| for (const [i, part] of currParts.entries()) { | |
| for (for let i = 0; i < currParts.length; i++) { | |
| const part = currParts[i]; |
I only suggest micro-optimizations in cases where it might matter (like vdom code), which is why I suggest it here. Avoiding the extra array allocations should theoretically be a slight perf boost. (Perhaps even measurable by the Tachometer tests.)
There was a problem hiding this comment.
I was wondering about this and wasn't sure if we would want to use a for loop instead.
I ran my benchmark with both versions and didn't see a difference but it's a rudimentary test.
I'll go with the for loop though as it'll be a perf boost!
Details
Fixes #3800
This PR fixes the issue of calling
traverseAndSetElementon every render by only traversing the parts instead of the entire subtree of the static node.I wrote a small perf benchmark to demonstrate the gains with deeply nested static parts, here.
The benchmark shows a roughly 17% perf improvement in this scenario.
The benchmark is not included in this PR as it is specific to this fix and I plan to include more benchmarks as part of the work in #3624.
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
GUS work item
W-15044923