-
Notifications
You must be signed in to change notification settings - Fork 393
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
perf: hoist static objects/arrays in templates #2589
Changes from all commits
e1b6050
9e9b3df
7bab41d
0158233
6e4524f
12683ae
1fa4262
29fdd8e
1e1d732
35e67ce
409cdee
c48f17e
c620819
e8dac64
3e2ce78
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -26,7 +26,7 @@ import { | |||||||||||||||
import { logError, logWarn } from '../shared/logger'; | ||||||||||||||||
import { invokeEventListener } from './invoker'; | ||||||||||||||||
import { getVMBeingRendered } from './template'; | ||||||||||||||||
import { EmptyArray, EmptyObject } from './utils'; | ||||||||||||||||
import { cloneAndOmitKey, EmptyArray, EmptyObject } from './utils'; | ||||||||||||||||
import { | ||||||||||||||||
appendVM, | ||||||||||||||||
getAssociatedVMIfPresent, | ||||||||||||||||
|
@@ -204,7 +204,11 @@ const ElementHook: Hooks<VElement> = { | |||||||||||||||
const { props } = vnode.data; | ||||||||||||||||
if (!isUndefined(props) && !isUndefined(props.innerHTML)) { | ||||||||||||||||
if (elm.innerHTML === props.innerHTML) { | ||||||||||||||||
delete props.innerHTML; | ||||||||||||||||
// Do a shallow clone since VNodeData may be shared across VNodes due to hoist optimization | ||||||||||||||||
vnode.data = { | ||||||||||||||||
...vnode.data, | ||||||||||||||||
props: cloneAndOmitKey(props, 'innerHTML'), | ||||||||||||||||
}; | ||||||||||||||||
Comment on lines
+207
to
+211
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When reaching this code path, there is no way the By the way, this block of code looks a little misplaced. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would kind of prefer to leave it as a clone. It seems unsafe for a developer to rely on quirks of the optimization algorithm. In the future, we may very well want to hoist: api_sanitize_html_content("static string") In fact, we are already caching the output on the lwc/packages/@lwc/template-compiler/src/__tests__/fixtures/directive-inner-html/valid/expected.js Lines 8 to 14 in 52e0a2b
So if we apply the hosting optimization, arguably we may not need the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. uff, uff! Additionally, the innerHTML should NOT be a regular property, it should have been a snabbdom module who's job is to compare two vnodes to determine whether or not the innerHTML needs to be set, rather than tricking the props module to not see the innerHTML for some cases when we know that it is the same. This is obviously not related to this PR, but needs to be fixed at some point. I might be missing something obvious here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||||||||
} else { | ||||||||||||||||
logWarn( | ||||||||||||||||
`Mismatch hydrating element <${elm.tagName.toLowerCase()}>: innerHTML values do not match for element, will recover from the difference`, | ||||||||||||||||
|
@@ -349,7 +353,7 @@ function addVNodeToChildLWC(vnode: VCustomElement) { | |||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
// [h]tml node | ||||||||||||||||
function h(sel: string, data: VElementData, children: VNodes): VElement { | ||||||||||||||||
function h(sel: string, data: Readonly<VElementData>, children: Readonly<VNodes>): VElement { | ||||||||||||||||
const vmBeingRendered = getVMBeingRendered()!; | ||||||||||||||||
if (process.env.NODE_ENV !== 'production') { | ||||||||||||||||
assert.isTrue(isString(sel), `h() 1st argument sel must be a string.`); | ||||||||||||||||
|
@@ -428,10 +432,10 @@ function ti(value: any): number { | |||||||||||||||
// [s]lot element node | ||||||||||||||||
function s( | ||||||||||||||||
slotName: string, | ||||||||||||||||
data: VElementData, | ||||||||||||||||
children: VNodes, | ||||||||||||||||
data: Readonly<VElementData>, | ||||||||||||||||
children: Readonly<VNodes>, | ||||||||||||||||
slotset: SlotSet | undefined | ||||||||||||||||
): VElement | VNodes { | ||||||||||||||||
): VElement | Readonly<VNodes> { | ||||||||||||||||
if (process.env.NODE_ENV !== 'production') { | ||||||||||||||||
assert.isTrue(isString(slotName), `s() 1st argument slotName must be a string.`); | ||||||||||||||||
assert.isTrue(isObject(data), `s() 2nd argument data must be an object.`); | ||||||||||||||||
|
@@ -791,8 +795,10 @@ function dc( | |||||||||||||||
// the new vnode key is a mix of idx and compiler key, this is required by the diffing algo | ||||||||||||||||
// to identify different constructors as vnodes with different keys to avoid reusing the | ||||||||||||||||
// element used for previous constructors. | ||||||||||||||||
data.key = `dc:${idx}:${data.key}`; | ||||||||||||||||
return c(sel, Ctor, data, children); | ||||||||||||||||
// Shallow clone is necessary here becuase VElementData may be shared across VNodes due to | ||||||||||||||||
nolanlawson marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||
// hoisting optimization. | ||||||||||||||||
const newData = { ...data, key: `dc:${idx}:${data.key}` }; | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The LWC compiler has been emitting unique keys to each VNode to speed up DOM diffing. The main downside with this is that it produces a lot of garbage in the generated template code. I think there is certainly a better way to handle this without adding more junk to the generated code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I was wondering what the key is for. If it's only to speed up DOM diffing, then what about removing it and relying on object equality? (Since objects are effectively immutable.) Or is the optimization still necessary? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keys are used to checking if vnodes are equal. it is primarily used whenever nodes are dynamic (if block, each block). In those cases, we can't use object equality as those VNodes can't be hoisted. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. keys are to denotate placeables for elements in the template, that's the way I think about them, and we use that information in multiple places to determine whether or not a vnode is the result of a particular placeable or another. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. another alternative here is to not use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. again, this is not a critical path since dynamic components are almost never used, just saying! :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm... I would personally prefer to keep |
||||||||||||||||
return c(sel, Ctor, newData, children); | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
/** | ||||||||||||||||
|
@@ -808,7 +814,7 @@ function dc( | |||||||||||||||
* - children that are produced by iteration | ||||||||||||||||
* | ||||||||||||||||
*/ | ||||||||||||||||
function sc(vnodes: VNodes): VNodes { | ||||||||||||||||
function sc(vnodes: Readonly<VNodes>): Readonly<VNodes> { | ||||||||||||||||
if (process.env.NODE_ENV !== 'production') { | ||||||||||||||||
assert.isTrue(isArray(vnodes), 'sc() api can only work with arrays.'); | ||||||||||||||||
} | ||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to
VNodes
being read-only now, I had to remove this line. AFAICT, it is not needed. I don't understand why we would need to set this toundefined
in theoldCh
array. The old children should just be garbage-collected, as they either belong to the old VNode:lwc/packages/@lwc/engine-core/src/framework/hooks.ts
Line 179 in c26277b
Or are replaced on the current VM:
lwc/packages/@lwc/engine-core/src/framework/vm.ts
Lines 418 to 419 in c26277b
lwc/packages/@lwc/engine-core/src/framework/vm.ts
Line 444 in c26277b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't yet fully understand the implications of removing this line here. The fact that it breaks no test is good news to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You CANNOT remove this line! :)
Basically, this line actuates when there is a new element who's key is found on the old vnodes, there are 2 possibilities:
The posibility of being in the same position is not possible in this branch of the code.
On top of that, there is another problem: "whether or not the tagName (sel) matches.
So, the problem is that if the sel matches, and the position is prior the new position (remember that this algo goes backward), since we are in a loop, there is a possibility that later in the game you encounter another element that must be inserted in the position marked by the old position, and the diffing algo tries to remove the element associated to the old vnodes.
My hunch is that this will result on the moved node to disappear from the DOM, and it is very hard to write a test that validate this assumption, but we can certainly try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@caridy Thanks for the explanation. I'm looking into it, and it turns out that this
else
block is never hit in either our Jest tests or Karma tests:lwc/packages/@lwc/engine-core/src/3rdparty/snabbdom/snabbdom.ts
Lines 127 to 146 in d93e296
In other words,
isUndef(idxInOld)
is always true. I'll try to write a test where it is false.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I managed to find a Karma test to hit this line of code (I'll open a separate PR to improve our test coverage). Based on the usage patterns, I think we can do this:
oldCh
directly, use a clone ([...oldCh]
) inside ofupdateDynamicChildren
. AFAICT,oldCh
is only used inside of that function – afterwards, it is discard and GC'ed.oldCh[idxInOld] = undefined as any;
).@caridy How does this sound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure the clone solves the problem, unless you always rely on the cloned array for the different paths of the algo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also closing via
[...oldCh]
will trigger the iterable protocol that is not ideal.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@caridy I'm saying we should do:
It solves the problem because the
oldCh
array isReadonly
– although only because of the empty array[]
, which may be shared between VNodes due to the optimization in this PR. (A non-empty array cannot be hoisted because it will contain VNodes, not static objects. This is why we only need a shallow clone, not a deep clone.)Can you elaborate? Are you concerned about performance? It seems to me that this code path is very infrequent, so I'm not really concerned about the perf implications there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'm fine with this approach. In locker code, we try to stay away from
[...array]
due to the perf implications of the iterables. cc @jdalton, but as you said, this is a very rare code path anyways.