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.

Sign up for GitHub

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

Jump to bottom

[Perf] Avoid React.Children.count traversal for empty state early return #15503

Merged
merged 7 commits into from
Nov 13, 2020
Merged

[Perf] Avoid React.Children.count traversal for empty state early return #15503

merged 7 commits into from
Nov 13, 2020

Conversation

KevinTCoughlin
Copy link
Member

Pull request checklist

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ yarn change

Description of changes

Avoid calls to React.Children.count which recursively traverses child components. The cost grows with component hierarchy. These cases were for early return null so they should be quick. In cases where children are not passed via props the value is undefined.

Text for example:

image

Focus areas to test

(optional)

Related: #15462 #15473

@msft-github-bot
Copy link
Contributor

Thanks for submitting this change, but due to work currently in progress to prepare master for our version 8 beta release, we're asking contributors to either wait a couple weeks to submit changes (if it's not urgent) or submit to the new 7.0 branch (if it's urgent). See #15222 for more details. Thank you for your contribution and understanding!

@KevinTCoughlin KevinTCoughlin changed the title Avoid React.Children.count traversal for empty state early return [Perf] Avoid React.Children.count traversal for empty state early return Oct 14, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 14, 2020 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit c3def6f:

Sandbox Source
Fluent UI Button Configuration
codesandbox-react-template Configuration
codesandbox-react-northstar-template Configuration

@KevinTCoughlin
Copy link
Member Author

These need to explicitly check === undefined. I'll push an iteration. The reason is <div>{0}</div> is treated as falsey otherwise (thanks unit tests).

@msft-github-bot
Copy link
Contributor

msft-github-bot commented Oct 14, 2020 •
edited
Loading

Perf Analysis

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 859 863 5000
BaseButtonCompat mount 953 1000 5000
Breadcrumb mount 160099 159739 5000
Checkbox mount 1725 1666 5000
CheckboxBase mount 1362 1354 5000
ChoiceGroup mount 5030 5035 5000
ComboBox mount 973 1005 1000
CommandBar mount 22259 22097 1000
ContextualMenu mount 6206 6452 1000
DefaultButtonCompat mount 1222 1227 5000
DetailsRow mount 3808 3732 5000
DetailsRowFast mount 3845 3736 5000
DetailsRowNoStyles mount 3582 3522 5000
Dialog mount 1546 1597 1000
DocumentCardTitle mount 1826 1854 1000
Dropdown mount 3560 3590 5000
FocusTrapZone mount 1916 1814 5000
FocusZone mount 1822 1802 5000
IconButtonCompat mount 1878 1962 5000
Label mount 348 351 5000
Layer mount 1933 1877 5000
Link mount 482 500 5000
MenuButtonCompat mount 1624 1581 5000
MessageBar mount 2027 2113 5000
Nav mount 3469 3536 1000
OverflowSet mount 1096 1101 5000
Panel mount 1466 1506 1000
Persona mount 898 899 1000
Pivot mount 1526 1461 1000
PrimaryButtonCompat mount 1366 1357 5000
Rating mount 8114 8252 5000
SearchBox mount 1414 1422 5000
Shimmer mount 2690 2774 5000
Slider mount 2013 1960 5000
SpinButton mount 5282 5207 5000
Spinner mount 426 428 5000
SplitButtonCompat mount 3407 3363 5000
Stack mount 526 544 5000
StackWithIntrinsicChildren mount 1712 1663 5000
StackWithTextChildren mount 5291 4996 5000
SwatchColorPicker mount 10839 10750 5000
TagPicker mount 2882 2902 5000
TeachingBubble mount 11959 12107 5000
Text mount 483 438 5000
TextField mount 1502 1513 5000
ThemeProvider mount 2115 2113 5000
ThemeProvider virtual-rerender 699 712 5000
Toggle mount 864 843 5000
button mount 606 597 5000
buttonNative mount 119 112 5000

Perf Analysis (Fluent)

Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🎯 Avatar.Fluent 0.49 0.56 0.87:1 2000 987
🦄 Button.Fluent 0.13 0.25 0.52:1 5000 670
🔧 Checkbox.Fluent 0.72 0.4 1.8:1 1000 717
🎯 Dialog.Fluent 0.18 0.24 0.75:1 5000 883
🔧 Dropdown.Fluent 3.11 0.43 7.23:1 1000 3108
🔧 Icon.Fluent 0.16 0.07 2.29:1 5000 813
🦄 Image.Fluent 0.09 0.13 0.69:1 5000 470
🔧 Slider.Fluent 1.66 0.47 3.53:1 1000 1663
🔧 Text.Fluent 0.08 0.04 2:1 5000 423
🦄 Tooltip.Fluent 0.12 0.92 0.13:1 5000 605

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
FlexMinimalPerf.default 374 346 1.08:1
RefMinimalPerf.default 264 244 1.08:1
Checkbox.Fluent 717 665 1.08:1
ButtonMinimalPerf.default 213 200 1.07:1
AttachmentMinimalPerf.default 197 185 1.06:1
GridMinimalPerf.default 450 423 1.06:1
ListNestedPerf.default 678 642 1.06:1
ButtonUseCssPerf.default 976 931 1.05:1
ChatWithPopoverPerf.default 528 503 1.05:1
DividerMinimalPerf.default 451 431 1.05:1
PopupMinimalPerf.default 773 737 1.05:1
AlertMinimalPerf.default 346 334 1.04:1
AvatarMinimalPerf.default 550 528 1.04:1
CarouselMinimalPerf.default 528 506 1.04:1
TextMinimalPerf.default 423 405 1.04:1
Dialog.Fluent 883 849 1.04:1
AnimationMinimalPerf.default 446 432 1.03:1
AttachmentSlotsPerf.default 1282 1244 1.03:1
ButtonOverridesMissPerf.default 1902 1841 1.03:1
CheckboxMinimalPerf.default 3121 3038 1.03:1
DialogMinimalPerf.default 876 850 1.03:1
CustomToolbarPrototype.default 4263 4119 1.03:1
TreeWith60ListItems.default 242 235 1.03:1
VideoMinimalPerf.default 727 703 1.03:1
Avatar.Fluent 987 956 1.03:1
Image.Fluent 470 457 1.03:1
Text.Fluent 423 409 1.03:1
Tooltip.Fluent 605 585 1.03:1
CardMinimalPerf.default 680 669 1.02:1
EmbedMinimalPerf.default 2171 2129 1.02:1
InputMinimalPerf.default 1464 1429 1.02:1
ListMinimalPerf.default 568 558 1.02:1
ListWith60ListItems.default 1005 984 1.02:1
PortalMinimalPerf.default 174 170 1.02:1
ButtonSlotsPerf.default 684 680 1.01:1
ButtonUseCssNestingPerf.default 1225 1207 1.01:1
DropdownManyItemsPerf.default 855 843 1.01:1
HeaderMinimalPerf.default 461 457 1.01:1
ImageMinimalPerf.default 468 464 1.01:1
ItemLayoutMinimalPerf.default 1491 1472 1.01:1
MenuMinimalPerf.default 971 963 1.01:1
ProviderMinimalPerf.default 1104 1088 1.01:1
SplitButtonMinimalPerf.default 4123 4094 1.01:1
StatusMinimalPerf.default 830 824 1.01:1
TextAreaMinimalPerf.default 577 570 1.01:1
Slider.Fluent 1663 1645 1.01:1
BoxMinimalPerf.default 424 424 1:1
LabelMinimalPerf.default 483 482 1:1
MenuButtonMinimalPerf.default 1721 1729 1:1
ProviderMergeThemesPerf.default 2123 2121 1:1
ReactionMinimalPerf.default 485 485 1:1
ToolbarMinimalPerf.default 1071 1075 1:1
TreeMinimalPerf.default 1030 1031 1:1
Button.Fluent 670 669 1:1
Icon.Fluent 813 810 1:1
ChatDuplicateMessagesPerf.default 453 459 0.99:1
FormMinimalPerf.default 525 528 0.99:1
HeaderSlotsPerf.default 913 920 0.99:1
LoaderMinimalPerf.default 807 814 0.99:1
RadioGroupMinimalPerf.default 515 519 0.99:1
SkeletonMinimalPerf.default 473 480 0.99:1
SliderMinimalPerf.default 1654 1677 0.99:1
TableManyItemsPerf.default 2488 2505 0.99:1
TooltipMinimalPerf.default 910 917 0.99:1
DropdownMinimalPerf.default 3090 3158 0.98:1
LayoutMinimalPerf.default 466 477 0.98:1
SegmentMinimalPerf.default 404 411 0.98:1
ChatMinimalPerf.default 673 694 0.97:1
ListCommonPerf.default 739 765 0.97:1
TableMinimalPerf.default 455 468 0.97:1
Dropdown.Fluent 3108 3218 0.97:1
AccordionMinimalPerf.default 184 191 0.96:1
IconMinimalPerf.default 748 785 0.95:1

@size-auditor
Copy link

size-auditor bot commented Oct 14, 2020 •
edited
Loading

Asset size changes

Project Bundle Baseline Size New Size Difference
office-ui-fabric-react fluentui-react-TeachingBubble 189.79 kB 189.776 kB BelowBaseline     -14 bytes
office-ui-fabric-react fluentui-react-Text 34.138 kB 34.122 kB BelowBaseline     -16 bytes
office-ui-fabric-react fluentui-react-Stack 38.479 kB 37.65 kB BelowBaseline     -829 bytes

ExceedsTolerance Over Tolerance (1024 B) ExceedsBaseline Over Baseline BelowBaseline Below Baseline New New Deleted  Removed 1 kB = 1000 B

Baseline commit: 8a3c81a98246558bdb80b05e1f1acf32eab34509 (build)

@khmakoto
Copy link
Member

Let's wait until after the snap period to merge or spin a v7 version of this PR if needed sooner.

ecraig12345
ecraig12345 reviewed Oct 24, 2020
View reviewed changes
Copy link
Member

@ecraig12345 ecraig12345 left a comment

Choose a reason for hiding this comment

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

Guessing this can go in now? Needs some minor change file updates and a merge with master.

change/@fluentui-react-internal-2020-10-13-16-47-27-keco-react-children-for-empty.json Outdated Show resolved Hide resolved
change/@uifabric-react-cards-2020-10-13-16-47-27-keco-react-children-for-empty.json Outdated Show resolved Hide resolved
@ecraig12345
Copy link
Member

Actually both the change files and the conflicts were easy to fix in the web UI, so I took care of that.

@KevinTCoughlin
Copy link
Member Author

Actually both the change files and the conflicts were easy to fix in the web UI, so I took care of that.

Thank you @ecraig12345 🙇‍♂️. @khmakoto can we merge this?

@ecraig12345 ecraig12345 merged commit 4a1eddf into microsoft:master Nov 13, 2020
@msft-github-bot
Copy link
Contributor

🎉@fluentui/react-cards@v1.0.0-beta.14 has been released which incorporates this pull request.:tada:

Handy links:

@msft-github-bot
Copy link
Contributor

🎉@fluentui/react-internal@v8.0.0-beta.12 has been released which incorporates this pull request.:tada:

Handy links:

SethDonohue pushed a commit to SethDonohue/fluentui that referenced this pull request Nov 23, 2020
@KevinTCoughlin
[Perf] Avoid React.Children.count traversal for empty state early ret…
ca7538c 
…urn (microsoft#15503)
Ridvanovskyy
Ridvanovskyy reviewed Feb 2, 2021
View reviewed changes
Copy link

@Ridvanovskyy Ridvanovskyy left a comment

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ridvanovskyy Ridvanovskyy Ridvanovskyy approved these changes

@khmakoto khmakoto khmakoto approved these changes

@ecraig12345 ecraig12345 ecraig12345 approved these changes

@xugao xugao xugao approved these changes

@dzearing dzearing Awaiting requested review from dzearing

@JustSlone JustSlone Awaiting requested review from JustSlone

@natalieethell natalieethell Awaiting requested review from natalieethell

Assignees

@khmakoto khmakoto

@tomi-msft tomi-msft

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@KevinTCoughlin @msft-github-bot @khmakoto @ecraig12345 @xugao @Ridvanovskyy @tomi-msft