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

Add performance measurement #9777

Merged

Conversation

sgraband
Copy link
Contributor

@sgraband sgraband commented Jul 26, 2021

What it does

This commit adds a script for performance measurement using the Chrome DevTools tracing.
Currently the startup time of the browser-app is being measured.
The script uses Puppeteer so other processes can be measured in the future as well.
For more information see the README.md in the scripts/performance directory.

This PR is intended as the first step to an eventual performance test suite which could also be integrated into the CI tooling.
See #9722 for more information about this topic.

Example output:

[Performance][Startup][1] Largest Contentful Paint (LCP): 2.95 seconds
[Performance][Startup][2] Largest Contentful Paint (LCP): 2.823 seconds
[Performance][Startup][3] Largest Contentful Paint (LCP): 2.829 seconds
[Performance][Startup][4] Largest Contentful Paint (LCP): 2.835 seconds
[Performance][Startup][5] Largest Contentful Paint (LCP): 2.778 seconds
[Performance][Startup][6] Largest Contentful Paint (LCP): 2.795 seconds
[Performance][Startup][7] Largest Contentful Paint (LCP): 2.772 seconds
[Performance][Startup][8] Largest Contentful Paint (LCP): 2.672 seconds
[Performance][Startup][9] Largest Contentful Paint (LCP): 2.706 seconds
[Performance][Startup][10] Largest Contentful Paint (LCP): 2.804 seconds
[Performance][Startup][MEAN] Largest Contentful Paint (LCP): 2.796 seconds
[Performance][Startup][STDEV] Largest Contentful Paint (LCP): 0.072 seconds

Contributed on behalf of STMicroelectronics

Signed-off-by: [email protected]

How to test

See the README on how you can run the script manually or run the performance:startup node script in the root directory to measure the startup time. You can also adjust the parameters to for example execute more iterations or to run the test headful.
Additionally, you can import the generated files in scripts/performance/profiles in the performance tab of the Chrome DevTools.

Review checklist

Reminder for reviewers

@sgraband sgraband force-pushed the performance-measurement branch 2 times, most recently from 5322deb to 9f56b25 Compare July 26, 2021 14:11
@vince-fugnitto vince-fugnitto added the performance issues related to performance label Jul 26, 2021
@sgraband sgraband force-pushed the performance-measurement branch 2 times, most recently from 21af9aa to a188660 Compare August 9, 2021 11:00
@sgraband sgraband force-pushed the performance-measurement branch 2 times, most recently from ab4de3b to 0a80b85 Compare September 7, 2021 07:48
.npmrc Outdated
@@ -0,0 +1 @@
scripts-prepend-node-path=true
Copy link
Contributor

Choose a reason for hiding this comment

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

What do we need this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this option the script showed a warning:

The node binary used for scripts is /tmp/yarn--1627025300882-0.06251617126804576/node but npm is using
/home/simon/.nvm/versions/node/v12.22.1/bin/node itself. Use the --scripts-prepend-node-path option to include the
path for the node binary npm was
executed with.

However, right now i cannot reproduce it. Should i remove it?

Copy link
Contributor

Choose a reason for hiding this comment

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

yarnpkg/yarn#6685 I haven't traced trough all that, but I think yarn is just making copies 🤷 I think we should remove it (or understand why it happens).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it.

scripts/performance/measure-performance.js Show resolved Hide resolved
scripts/performance/measure-performance.js Outdated Show resolved Hide resolved
scripts/performance/README.md Outdated Show resolved Hide resolved
scripts/performance/README.md Outdated Show resolved Hide resolved
scripts/performance/measure-performance.js Outdated Show resolved Hide resolved
scripts/performance/measure-performance.js Show resolved Hide resolved
scripts/performance/measure-performance.js Outdated Show resolved Hide resolved
scripts/performance/measure-performance.js Outdated Show resolved Hide resolved
scripts/performance/measure-performance.js Show resolved Hide resolved
@tsmaeder
Copy link
Contributor

tsmaeder commented Sep 7, 2021

@sgraband thanks for this contribution: it's important that we keep track of startup performance. I have a bunch of suggestions/comments which are mostly code style issues.

@sgraband sgraband force-pushed the performance-measurement branch 2 times, most recently from 3160b56 to f1ce9ca Compare September 9, 2021 10:28
@sgraband
Copy link
Contributor Author

@tsmaeder I rebased the changes to master. Could you have another look?

@colin-grant-work
Copy link
Contributor

@sgraband, it looks like the builds are failing here because running yarn on this commit changes the yarn.lock. You may need to commit those changes so that the builds are satisfied that our yarn.lock is up to date.

@sgraband
Copy link
Contributor Author

sgraband commented Oct 4, 2021

@colin-grant-work rebasing seemed to do the trick.

Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

This script is performing correctly for me and seems like a reasonable basis for judging changes in startup performance for the application. @tsmaeder, do you have any additional comments?

@tsmaeder
Copy link
Contributor

tsmaeder commented Oct 5, 2021

@colin-grant-work nothing to prevent merging.

This commit adds a script for performance measurement using the Chrome DevTools tracing.
Currently the startup time of the browser-app is being measured.
The script uses Puppeteer so other processes can be measured in the future as well.
For more information see the README in the scripts/performance directory.

Contributed on behalf of STMicroelectronics

Signed-off-by: Simon Graband <[email protected]>
@sgraband
Copy link
Contributor Author

sgraband commented Oct 6, 2021

Rebased and removed the .npmrc file.

@colin-grant-work colin-grant-work merged commit f75bcde into eclipse-theia:master Oct 6, 2021
@vince-fugnitto vince-fugnitto added this to the 1.19.0 milestone Oct 28, 2021
@vince-fugnitto vince-fugnitto added logging issues related to logging metrics issues related to metrics and logging labels Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logging issues related to logging metrics issues related to metrics and logging performance issues related to performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants