Skip to content

Reduce reads of dom.nextSibling#2294

Merged
JoviDeCroock merged 5 commits into
masterfrom
diffchildren-reduce-nextSibling-calls
Feb 1, 2020
Merged

Reduce reads of dom.nextSibling#2294
JoviDeCroock merged 5 commits into
masterfrom
diffchildren-reduce-nextSibling-calls

Conversation

@andrewiggins

@andrewiggins andrewiggins commented Jan 30, 2020

Copy link
Copy Markdown
Member

While exploring ways to use getDomSibling to replace nextSibling in diffChildren, I discovered that _lastDomChild should really be _lastDomChildSibling. The only thing our code uses _lastDomChild for is to determine what DOM node the diff should continue with after diffing a Component/Fragment.

However when Components/Fragments are nested (like the example below) each Component reads it's child's _lastDomChild property and reads the nextSibling property. If the Components are the last child of their parent's (like the example below), then this nextSibling read is redundant to the read that it's child already performed. Note: it's only useless when the Component is the last child because the result of oldDom = newDom.nextSibling is thrown away. In diffChildren, oldDom is only used to determine what DOM the diff should use for siblings. When the Component doesn't have any siblings, the value of oldDom is thrown away and _lastDomChild is used instead in the parent.

<Provider1>
  <Provider2>
    <Child>
      <div />
    </Child>
  </Provider2>
</Provider1>

In other words, what we really want to bubble up while diffing Components/Fragments isn't the last DOM child of the Component, but what DOM should the diff continue with. So this PR changes _lastDomChild to _lastDomChildSibling.

I also used this change to precalculate what oldDom should be in situations where we know it without having to read nextSibling

@github-actions

github-actions Bot commented Jan 30, 2020

Copy link
Copy Markdown

Size Change: +40 B (0%)

Total Size: 38.2 kB

Filename Size Change
debug/dist/debug.js 3.08 kB -1 B
debug/dist/debug.module.js 3.06 kB -2 B (0%)
dist/preact.js 3.7 kB +10 B (0%)
dist/preact.min.js 3.7 kB +10 B (0%)
dist/preact.module.js 3.72 kB +12 B (0%)
dist/preact.umd.js 3.77 kB +11 B (0%)
ℹ️ View Unchanged
Filename Size Change
compat/dist/compat.js 2.96 kB 0 B
compat/dist/compat.module.js 2.99 kB 0 B
compat/dist/compat.umd.js 3.01 kB 0 B
debug/dist/debug.umd.js 3.14 kB 0 B
devtools/dist/devtools.js 175 B 0 B
devtools/dist/devtools.module.js 185 B 0 B
devtools/dist/devtools.umd.js 252 B 0 B
hooks/dist/hooks.js 1.06 kB 0 B
hooks/dist/hooks.module.js 1.08 kB 0 B
hooks/dist/hooks.umd.js 1.12 kB 0 B
test-utils/dist/testUtils.js 390 B 0 B
test-utils/dist/testUtils.module.js 392 B 0 B
test-utils/dist/testUtils.umd.js 469 B 0 B

compressed-size-action

@coveralls

coveralls commented Jan 30, 2020

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.1%) to 99.808% when pulling 49962e7 on diffchildren-reduce-nextSibling-calls into 82e870a on master.

Comment thread debug/src/debug.js
parentVNode.type !== 'tfoot' &&
parentVNode.type !== 'tbody' &&
parentVNode.type !== 'table')
parentVNode.type !== 'thead' &&

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how Prettier formatted this for me 🤷‍♂

Comment thread src/create-element.js
Comment thread src/diff/children.js

outer: if (oldDom == null || oldDom.parentNode !== parentDom) {
parentDom.appendChild(newDom);
nextDom = null;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are adding newDom to the end of parentDom, newDom.nextSibling will always be null. So instead of going through the DOM APIs to get this, I'm precalculating it here so save us a DOM read.

Comment thread src/diff/children.js
}
}
parentDom.insertBefore(newDom, oldDom);
nextDom = oldDom;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are inserting newDom before oldDom, newDom.nextSibling will always be oldDom so I'm precalculating this now to save us a DOM read.

Comment thread debug/src/debug.js Outdated
let Foo = <div />;
let fn = () => render(h(<Foo />), scratch);
expect(fn).to.throw(/createElement/);
expect(fn).to.throw(/JSX twice/);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was giving a false positive because my change caused it to throw a different error that contained createElement when it should've been throwing this error. Caught this by looking at the code coverage and noticing that the line under the condition I changed was no longer covered lol.

@JoviDeCroock JoviDeCroock left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great find Andre!

@JoviDeCroock JoviDeCroock merged commit 0fb3b13 into master Feb 1, 2020
@JoviDeCroock JoviDeCroock deleted the diffchildren-reduce-nextSibling-calls branch February 1, 2020 00:10
porfirioribeiro pushed a commit to porfirioribeiro/preact that referenced this pull request Feb 3, 2020
* Reduce calls to nextSibling for nested Fragments/Components (+5 B)

* Rename _lastDomChild to _lasDomChildSibling (-1 B)

* Precalculate nextDom for append and inserts (+6 B)

* Fix debug vnode type checking

Co-authored-by: Jovi De Croock <decroockjovi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants