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 Test: Flamegraphs #9550

Merged
merged 17 commits into from
Jun 24, 2019
Merged

Perf Test: Flamegraphs #9550

merged 17 commits into from
Jun 24, 2019

Conversation

JasonGore
Copy link
Member

@JasonGore JasonGore commented Jun 21, 2019

Pull request checklist

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

Description of changes

This PR replaces the existing perf test table with a new process that generates profiles using Puppeteer / V8 and uses those profiles to generate flamegraphs. All sample counts and flamegraphs will be added as comments to the PR, just as existing perf test package operates.

There are many improvements to come, but for now this first iteration provides the following:

  1. Autogenerated flamegraphs will show improvements / regressions by PR as well as an indication of bottlenecks.
  2. Sample count are still subject to +- 30% variance, but through the use of V8 flags, are more consistent, at least when run locally.

Hopefully examples will show up when this PR builds.

Microsoft Reviewers: Open in CodeFlow


// A high number of iterations are needed to get visualization of lower level calls that are infrequently hit by ticks.
// TODO: change to 5000
const iterations = 10;
Copy link
Member

Choose a reason for hiding this comment

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

Worth defaulting to 5000 but allow for CLI arg?

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly. I think it'd also be useful to control which scenarios are running as most people who run locally will most likely only care about one or two.

@JasonGore
Copy link
Member Author

I had to make this PR to test PR integration and work with Ken to fix up a deploy issue. Will be doing more cleanup along the way.

@size-auditor
Copy link

size-auditor bot commented Jun 21, 2019

Size Auditor did not detect a change in bundle size for any component!

@msft-github-bot
Copy link
Contributor

msft-github-bot commented Jun 24, 2019

Component Perf Analysis:

Scenario Master Samples * PR Samples *
BaseButton 81 819
BaseButtonNew 72 3520
DefaultButton 71 1094
DefaultButtonNew 70 3047
DetailsRow 70 8048
DetailsRowNoStyles 68 6051
DocumentCardTitle 69 41552
MenuButton 73 1928
MenuButtonNew 65 6107
PrimaryButton 66 1334
PrimaryButtonNew 73 3439
SplitButton 69 3690
SplitButtonNew 76 13111
Toggle 64 1900
ToggleNew 67 2459
button 71 73
* 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
Copy link
Member Author

Yay, flamegraph links are working. Master in this case is still old perf-test, so results won't be comparable until after this PR is merged.

Copy link
Member

@kenotron kenotron left a comment

Choose a reason for hiding this comment

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

okay, minor nits about where to put TODO notes and perf notes, but looks after realizing the outfileMaster != outfilePR

apps/perf-test/perfLearnings.txt Outdated Show resolved Hide resolved
apps/perf-test/src/Measurer.tsx Outdated Show resolved Hide resolved
apps/perf-test/tasks/perf-test.js Show resolved Hide resolved
Copy link
Contributor

@cliffkoh cliffkoh left a comment

Choose a reason for hiding this comment

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

For the bunch of things I looked at, looks good to me. Did not read the new perf-test.js line by line - primarily looked at overall structure and high level approach.

@JasonGore JasonGore merged commit 771460b into microsoft:master Jun 24, 2019
This was referenced Jun 25, 2019
gingeroun pushed a commit to gingeroun/office-ui-fabric-react that referenced this pull request Jul 5, 2019
* Port changes from 6.0 to master.

* WIP: working on scenario list

* scenario list go!

* fix shrinkwrap

* scenariolist fix

* hooking it up with perf-test

* Clean up flamegraphs regressed from webpack server to local file conversion. Fix React render consumption caused by use of fragments.

* Add master reference tests. Generate results blob. Add all scenarios from perf-test app.

* Fix master URL.

* Fix shrinkwrap.

* Add path for perf-tests task since it was removed from root config.

* Fix perf-test command.

* Copy perf test results to PR deploy site. Add BaseButtonNew case.

* Tweak output.

* Final cleanup.

* Disable puppeteer timeout.
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants