Button: Removing Stack usage to improve perf #9516

khmakoto · 2019-06-19T23:06:20Z

Pull request checklist

Addresses an existing issue: Fixes #0000
Include a change request file using $ npm run change

Description of changes

This PR removes Stack in favor of emulating it with flex-box styles in Button, MenuButton and SplitButton, in order to improve the performance of the component.

To check for the perf changes I pulled the changes @JasonGore has been working on with puppeteer and flamebearer to get sample counts instead of time counts. These scenarios render the components 5000 times. I ran each scenario 10 times to get an average number and account for variants.

Below are the sample counts for each of the variants before and after the Stack removal.

DefaultButton:

Scenario	Run 1	Run 2	Run 3	Run 4	Run 5	Run 6	Run 7	Run 8	Run 9	Run 10	Average
Before Stack removal	1451	1515	1473	1454	1498	1439	1531	1498	1604	1508	1497.1
After Stack removal	1152	1039	1100	1132	1143	1169	1099	1165	1156	1184	1133.9

Improvement of 24.26%.

PrimaryButton:

Scenario	Run 1	Run 2	Run 3	Run 4	Run 5	Run 6	Run 7	Run 8	Run 9	Run 10	Average
Before Stack removal	1459	1541	1427	1550	1521	1492	1492	1546	1630	1534	1519.2
After Stack removal	1093	1162	1139	1207	1182	1201	1167	1166	1154	1200	1167.1

Improvement of 23.18%.

MenuButton:

Scenario	Run 1	Run 2	Run 3	Run 4	Run 5	Run 6	Run 7	Run 8	Run 9	Run 10	Average
Before Stack removal	2392	2413	2514	2494	2434	2448	2406	2455	2537	2421	2451.4
After Stack removal	2150	2127	2137	2128	2118	2146	2197	2203	2206	2161	2157.3

Improvement of 12.00%.

SplitButton:

Scenario	Run 1	Run 2	Run 3	Run 4	Run 5	Run 6	Run 7	Run 8	Run 9	Run 10	Average
Before Stack removal	5088	5024	4988	5015	5102	5026	5031	5117	5110	5098	5059.9
After Stack removal	3810	3847	3910	3771	3810	3656	3709	3724	3847	3803	3788.7

_Improvement of 25.12%.

Microsoft Reviewers: Open in CodeFlow

…c-react into buttonStackUsage

size-auditor · 2019-06-19T23:34:49Z

Bundle test	Size (minified)	Diff from master
experiments-Button	85.772 kB	-2.942 kB

Exceeds Tolerance Exceeds Baseline Below Baseline 1 kB = 1000 bytes

khmakoto

A lot of files were touched because of prettier. I'm marking with comments those files where no logic was changed and only prettier changes were involved.

packages/office-ui-fabric-react/src/components/Coachmark/Coachmark.base.tsx

packages/office-ui-fabric-react/src/components/ColorPicker/ColorPicker.base.tsx

packages/office-ui-fabric-react/src/components/Dropdown/Dropdown.base.tsx

packages/office-ui-fabric-react/src/components/KeytipLayer/KeytipTree.ts

packages/office-ui-fabric-react/src/utilities/decorators/withViewport.tsx

packages/merge-styles/src/IStyleSet.ts

apps/fabric-website/src/pages/Controls/SeparatorPage/docs/android/SeparatorImplementation.md

apps/fabric-website/src/pages/Controls/ListCellsPage/ListCellsPage.tsx

apps/fabric-website/src/pages/Controls/DrawerPage/DrawerPage.tsx

packages/example-app-base/src/utilities/createApp.tsx

msft-github-bot · 2019-06-20T00:37:57Z

Component Perf Analysis:

Scenario	Master Samples *	PR Samples *
BaseButton	883	895
BaseButtonNew	3734	2536
DefaultButton	1175	1175
DefaultButtonNew	3241	2039
DetailsRow	8409	8535
DetailsRowNoStyles	6357	6298
DocumentCardTitle	44342	44246
MenuButton	2068	2078
MenuButtonNew	6473	4910
PrimaryButton	1391	1361
PrimaryButtonNew	3658	2427
SplitButton	3845	3847
SplitButtonNew	14086	9225
Toggle	2037	2018
ToggleNew	2553	2485
button	81	70

* Sample counts can vary by up to 30% and shouldn't be used solely for determining regression. Flamegraph links are provided to give a hint on deltas introduced by PRs and potential bottlenecks.

JasonGore · 2019-06-24T15:58:04Z

packages/experiments/src/components/Button/Button.styles.ts

@@ -1,6 +1,7 @@
 import { IButtonComponent, IButtonStylesReturnType, IButtonTokenReturnType } from './Button.types';
 import { getFocusStyle, getGlobalClassNames, FontWeights, HighContrastSelector } from '../../Styling';
 import { IsFocusVisibleClassName } from '../../Utilities';
+import { parseGap } from 'office-ui-fabric-react/lib/components/Stack/StackUtils';


This path doesn't seem right for a component styles file.

Yeah, I was hoping to get your and @dzearing input here. Basically, I didn't want to rewrite the gap parsing function that lives inside Stack and the easiest way was to get it from this path, but I agree that it looks wrong in the context of a component styles file.

One thing we could do is make Stack export the StackUtils file which would allow us to use parseGap in other places just importing it from office-ui-fabric-react but it would increase the bundle-size of the Stack component.

I think this is ok for now since this is an experimental component using an OUFR utility, but it should be changed once it's promoted to a relative path.

packages/experiments/src/components/Button/Button.types.tsx

packages/experiments/src/components/Button/SplitButton/SplitButton.types.tsx

khmakoto · 2019-06-24T22:00:28Z

packages/office-ui-fabric-react/src/components/FocusZone/FocusZone.tsx

@@ -224,7 +224,7 @@ export class FocusZone extends React.Component<IFocusZoneProps, {}> implements I
          // root props has been deprecated and should get removed.
          // it needs to be marked as "any" since root props expects a div element, but really Tag can
          // be any native element so typescript rightly flags this as a problem.
-          ...(rootProps as any)
+          ...rootProps as any


Prettier change.

JasonGore · 2019-06-25T15:44:11Z

PR Samples is off because the test ran against an older version of the perf-test script from a previous build in this PR due to a missed copy step in the CI workflow. I'll make a PR to fix and then merge master into this PR again.

msft-github-bot · 2019-06-26T22:50:04Z

Hello @khmakoto!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msft-github-bot`) and give me an instruction to get started! Learn more here.

msft-github-bot · 2019-06-28T21:21:20Z

🎉office-ui-fabric-react@v7.6.1 has been released which incorporates this pull request.:tada:

Handy links:

* Button: Removing Stack usage from slots and view to improve perf. * Adding change files for packages that were changed because of prettier. * Deleting perf log files. * Undoing prettier changes. * Removing unused change files. * Removing BaseButton changes from Actionable PR.

khmakoto added 3 commits June 18, 2019 16:34

Button: Removing Stack usage from slots and view to improve perf.

3e27afe

Fixing merge conflict.

a737912

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

e615bc2

…c-react into buttonStackUsage

khmakoto added Component: Button Status: Do Not Merge 🚫 Package: experiments labels Jun 19, 2019

khmakoto requested review from dzearing, joschect and JasonGore June 19, 2019 23:06

khmakoto requested review from ecraig12345, Jahnp, jordandrako, kenotron, leddie24 and srideshpande as code owners June 19, 2019 23:06

msft-github-bot assigned natalieethell Jun 19, 2019

khmakoto changed the title ~~Button stack usage~~ Button: Removing Stack usage to improve perf Jun 19, 2019

micahgodbolt mentioned this pull request Jun 19, 2019

Refactor: Button #6134

Closed

37 tasks

Adding change files for packages that were changed because of prettier.

5c9dff9

khmakoto commented Jun 20, 2019

View reviewed changes

khmakoto added 3 commits June 20, 2019 12:55

Deleting perf log files.

6cc1b76

Undoing prettier changes.

81e32b6

Removing unused change files.

acf0b50

khmakoto removed request for kenotron, Jahnp, leddie24, ecraig12345 and jordandrako June 20, 2019 20:50

khmakoto removed the request for review from srideshpande June 20, 2019 20:50

JasonGore reviewed Jun 24, 2019

View reviewed changes

Fixing merge conflicts.

640e2c9

khmakoto commented Jun 24, 2019

View reviewed changes

Removing BaseButton changes from Actionable PR.

dd8ecc2

khmakoto removed the Status: Do Not Merge 🚫 label Jun 24, 2019

JasonGore mentioned this pull request Jun 25, 2019

Perf Test: Add missing copy step #9571

Merged

2 tasks

Merge branch 'master' into buttonStackUsage

64c5ac8

JasonGore approved these changes Jun 26, 2019

View reviewed changes

khmakoto added the AutoMerge label Jun 26, 2019

msft-github-bot merged commit 7f01905 into microsoft:master Jun 26, 2019

khmakoto deleted the buttonStackUsage branch June 26, 2019 22:52

microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Button: Removing Stack usage to improve perf #9516

Button: Removing Stack usage to improve perf #9516

khmakoto commented Jun 19, 2019 •

edited

Loading

size-auditor bot commented Jun 19, 2019 •

edited

Loading

khmakoto left a comment

msft-github-bot commented Jun 20, 2019 •

edited

Loading

JasonGore Jun 24, 2019

khmakoto Jun 24, 2019

JasonGore Jun 26, 2019

khmakoto Jun 24, 2019

JasonGore commented Jun 25, 2019 •

edited

Loading

msft-github-bot commented Jun 26, 2019

msft-github-bot commented Jun 28, 2019

Button: Removing Stack usage to improve perf #9516

Button: Removing Stack usage to improve perf #9516

Conversation

khmakoto commented Jun 19, 2019 • edited Loading

Pull request checklist

Description of changes

Microsoft Reviewers: Open in CodeFlow

size-auditor bot commented Jun 19, 2019 • edited Loading

khmakoto left a comment

Choose a reason for hiding this comment

msft-github-bot commented Jun 20, 2019 • edited Loading

JasonGore Jun 24, 2019

Choose a reason for hiding this comment

khmakoto Jun 24, 2019

Choose a reason for hiding this comment

JasonGore Jun 26, 2019

Choose a reason for hiding this comment

khmakoto Jun 24, 2019

Choose a reason for hiding this comment

JasonGore commented Jun 25, 2019 • edited Loading

msft-github-bot commented Jun 26, 2019

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msft-github-bot) and give me an instruction to get started! Learn more here.

msft-github-bot commented Jun 28, 2019

khmakoto commented Jun 19, 2019 •

edited

Loading

size-auditor bot commented Jun 19, 2019 •

edited

Loading

msft-github-bot commented Jun 20, 2019 •

edited

Loading

JasonGore commented Jun 25, 2019 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msft-github-bot`) and give me an instruction to get started! Learn more here.