Skip to content
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: avoid using a stack in traverseAndSetElements #4181

Merged
merged 4 commits into from
Apr 29, 2024

Conversation

nolanlawson
Copy link
Contributor

@nolanlawson nolanlawson commented Apr 26, 2024

Details

This improves the js-framework-benchmark's create-10k test by 8-10% in Chrome/Firefox and 4-9% in Safari. It does so by avoiding either recursion or a stack (array) by using a loop with firstChild/nextSibling/parentNode.

Click to see benchmark results

Screenshot 2024-04-26 at 1 58 36 PM
Screenshot 2024-04-26 at 3 23 24 PM
Screenshot 2024-04-26 at 3 19 08 PM

I was kind of concerned that adding parentNode would cause a perf regression in synthetic shadow (due to patches), but I tested manually in synthetic shadow (nolanlawson@4536c23), and it's faster there as well:

Click to see

Screenshot 2024-04-26 at 1 46 29 PM

I also tried using a TreeWalker (nolanlawson@303e7b7), since this is what Lit does. However, I found that the TreeWalker is ~0.5% slower than the loop:

Click to see

Screenshot 2024-04-26 at 3 09 55 PM

(TreeWalker is this-change, loop is tip-of-tree)

We may still want to consider the TreeWalker someday, due to the possible benefits in synthetic shadow (which does not patch TreeWalker at all), and because it's slightly less code.

One major downside, though, is that we'd need to implement a TreeWalker shim for @lwc/engine-server. For that reason, and because this is the optimal solution for js-framework-benchmark, I'm proposing this for now.

Does this pull request introduce a breaking change?

  • 😮‍💨 No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • 🤞 No, it does not introduce an observable change.

@nolanlawson nolanlawson requested a review from a team as a code owner April 26, 2024 22:35
Copy link
Contributor

@jye-sf jye-sf left a comment

Choose a reason for hiding this comment

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

This is really neat! It makes me wonder whether we can apply this to some of the other depth-first traversals we do in the engine (and/or whether those traversals are hot enough to warrant the effort).

while (isNull((sibling = nextSibling(node)))) {
if (process.env.NODE_ENV !== 'production') {
// We should never traverse up to the root. We should exit early due to numFoundParts === numParts.
assert.isFalse(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a specific situation you're thinking about that warrants this assertion, and should we have a test case for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not really possible to hit, but I'm concerned we could recurse up past the root if (somehow) there were more parts to be found. This happened in the past when we messed up and forgot to include comment nodes when preserve-comments is on.

@nolanlawson
Copy link
Contributor Author

It makes me wonder whether we can apply this to some of the other depth-first traversals we do in the engine (and/or whether those traversals are hot enough to warrant the effort).

Yep, another one is recursivelySetShadowResolver in synthetic-shadow. We could use this algorithm there too instead of using recursion.

@nolanlawson nolanlawson merged commit c1b33a8 into master Apr 29, 2024
10 checks passed
@nolanlawson nolanlawson deleted the nolan/optimize-traverse-2 branch April 29, 2024 22:43
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.

None yet

2 participants