perf(engine-core): optimize slot iteration #3166
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nolanlawson
commented
Nov 14, 2022
| // Evaluate in the scope of the slot content's owner | ||
| // if a slotset is provided, there will always be an owner. The only case where owner is | ||
| // undefined is for root components, but root components cannot accept slotted content | ||
| setVMBeingRendered(slotset.owner!); | ||
| try { | ||
| scopedSlotChildren = vnode.factory(data.slotData); | ||
| ArrayPush.apply(newChildren, vnode.factory(data.slotData) as VNode[]); |
There was a problem hiding this comment.
I considered isolating just the factory() call here for readability, but it adds quite a bit of extra code:
const vmBeingRenderedInception = getVMBeingRendered();
let scopedSlotChildren: VNodes | undefined;
setVMBeingRendered(slotset.owner!);
try {
scopedSlotChildren = vnode.factory(data.slotData);
} finally {
setVMBeingRendered(vmBeingRenderedInception);
}
if (!isUndefined(scopedSlotChildren)) {
ArrayPush.apply(newChildren, scopedSlotChildren as VNode[]);
}
There was a problem hiding this comment.
The return value from the factory need to be spread out and not inserted as-is.
There was a problem hiding this comment.
@ravijayaramappa This is Array.push.apply, not ArrayPush.call. So it is being spread out. 🙂
nolanlawson
commented
Nov 14, 2022
| } | ||
| // If the passed slot content is factory, evaluate it and add the produced vnodes | ||
| if (assignedNodeIsScopedSlot) { | ||
| const vmBeingRenderedInception = getVMBeingRendered(); | ||
| let scopedSlotChildren: VNodes = []; |
There was a problem hiding this comment.
Avoiding creating an extra [] here.
nolanlawson
commented
Nov 14, 2022
| if (vnode) { | ||
| const newChildren: VNode[] = []; | ||
| const slotAssignments = slotset.slotAssignments[slotName]; | ||
| for (let i = 0; i < slotAssignments.length; i++) { |
There was a problem hiding this comment.
I didn't check what the fastest way to iterate is (e.g. compared to for (const vnode of slotAssignments) {}. But we do use this pattern in lots of places in the code.
divmain
approved these changes
Nov 14, 2022
ravijayaramappa
approved these changes
Nov 14, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
It turns out that 2ada663 and e983223 introduced a perf regression. The
dom-ss-slot-update-slotted-content-5kbenchmark regressed 16% comparingmasterto 240. The culprit is thisreducefunction:lwc/packages/@lwc/engine-core/src/framework/api.ts
Line 192 in 6376013
This PR improves the benchmark by 11-12% (comparing this PR to
master):Improvements:
Array.prototype.reduceArrayPushinstead ofArrayConcat(ArrayConcatcreates a brand-new copy of the array, which we don't strictly need)Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
GUS work item
W-12028575