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

Improved buildTree. #6325

Merged
merged 8 commits into from
Sep 11, 2024
Merged

Improved buildTree. #6325

merged 8 commits into from
Sep 11, 2024

Conversation

seanparsons
Copy link
Contributor

@seanparsons seanparsons commented Sep 5, 2024

Problem:
buildTree is another function on our hot path for any interaction and can often cost around 2ms of frame time.

Fix:
By combining the logic from buildTreeRecursive_MUTATE and getChildrenPaths, we are able to turn the logic for adding an item to the tree into a function which is executed for the two cases where paths are added. Then it's possible to reshape the recursive call into a loop which avoids some unnecessary array construction. Along with this some tweaks were made to avoid some repetition in the logic which checks the types of the elements as they were working from the ElementInstanceMetadata each time for 5 different cases.

To help with ordering and to avoid having to do a lot of re-ordering the children field of ElementPathTree is now split into two distinct fields. As the inner children (from within the component) should be listed ahead of the props children (from within the JSX element in the code).

Results:
Benchmarks before:

Running "buildTree - deeply nested elements" suite...
  deeply nested elements:
    2,605 ops/s, ±2.16%
Running "buildTree - very wide elements" suite...
  very wide elements:
    9,462 ops/s, ±1.88%

Benchmarks after:

Running "buildTree - deeply nested elements" suite...
  deeply nested elements:
    5,431 ops/s, ±1.39%
Running "buildTree - very wide elements" suite...
  very wide elements:
    23,598 ops/s, ±0.73%

The above translates to buildTree taking about 0.5ms to run consistently.

Commit Details:

  • Reworked buildTree_MUTATE to build the tree from the root downwards, combining it with some of the logic taken from getChildrenPaths.
  • Further improvement to isDescendantOf replacing the previous implementation.
  • getReorderedPaths now avoids a lot of repeated logic for checking the element type.
  • Broke apart ElementPathTree.children into two separate fields
    to make for easier ordering.
  • Added getElementPathTreeChildren and forEachElementPathTreeChild utility functions.
  • buildTree_MUTATE now adds to the two different children fields based
    on the element path representing inner children over property children.
  • getReorderedIndexInPaths further reworked to handle shifting by the siblings.
  • Fixed some tests that had expressions incorrectly ordered.

Manual Tests:
I hereby swear that:

  • I opened a hydrogen project and it loaded
  • I could navigate to various routes in Preview mode

Fixes #6324

- Reworked `buildTree_MUTATE` to build the tree from the root downwards,
  combining it with some of the logic taken from `getChildrenPaths`.
- Further improvement to `isDescendantOf` replacing the previous implementation.
- `getReorderedPaths` now avoids a lot of repeated logic for checking the element type.
Copy link
Contributor

github-actions bot commented Sep 5, 2024

Try me

Copy link

relativeci bot commented Sep 5, 2024

#14182 Bundle Size — 62.62MiB (~+0.01%).

e0d58e3(current) vs 916e8b9 master#14176(baseline)

Warning

Bundle contains 70 duplicate packages – View duplicate packages

Bundle metrics  Change 2 changes Regression 1 regression
                 Current
#14182
     Baseline
#14176
Regression  Initial JS 45.74MiB(~+0.01%) 45.74MiB
No change  Initial CSS 0B 0B
Change  Cache Invalidation 21.67% 21.65%
No change  Chunks 30 30
No change  Assets 33 33
No change  Modules 4385 4385
No change  Duplicate Modules 523 523
No change  Duplicate Code 31.65% 31.65%
No change  Packages 472 472
No change  Duplicate Packages 70 70
Bundle size by type  Change 2 changes Regression 1 regression Improvement 1 improvement
                 Current
#14182
     Baseline
#14176
Regression  JS 62.61MiB (~+0.01%) 62.6MiB
Improvement  HTML 11.12KiB (-0.32%) 11.16KiB

Bundle analysis reportBranch performance/improved-buildtreeProject dashboard


Generated by RelativeCIDocumentationReport issue

@seanparsons seanparsons marked this pull request as draft September 6, 2024 09:58
- Broke apart `ElementPathTree.children` into two separate fields
  to make for easier ordering.
- Added `getElementPathTreeChildren` and `forEachElementPathTreeChild` utility functions.
- `buildTree_MUTATE` now adds to the two different children fields based
  on the element path representing inner children over property children.
- `getReorderedIndexInPaths` further reworked to handle shifting by the siblings.
- Fixed some tests that had expressions incorrectly ordered.
@seanparsons seanparsons marked this pull request as ready for review September 10, 2024 14:55
@seanparsons seanparsons merged commit 73b4df7 into master Sep 11, 2024
20 checks passed
@seanparsons seanparsons deleted the performance/improved-buildtree branch September 11, 2024 11:49
liady pushed a commit that referenced this pull request Dec 13, 2024
- Reworked `buildTree_MUTATE` to build the tree from the root downwards,
combining it with some of the logic taken from `getChildrenPaths`.
- Further improvement to `isDescendantOf` replacing the previous
implementation.
- `getReorderedPaths` now avoids a lot of repeated logic for checking
the element type.
- Broke apart `ElementPathTree.children` into two separate fields
  to make for easier ordering.
- Added `getElementPathTreeChildren` and `forEachElementPathTreeChild`
utility functions.
- `buildTree_MUTATE` now adds to the two different children fields based
on the element path representing inner children over property children.
- `getReorderedIndexInPaths` further reworked to handle shifting by the
siblings.
- Fixed some tests that had expressions incorrectly ordered.
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