-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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 Electron start-up performance script #10442
Add Electron start-up performance script #10442
Conversation
bbfad6c
to
4ff4f95
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the most part, this works as advertized. I did run into one oddity. It's possible to get into a state where running the electron app fails because electron-rebuild erroneously believes that native modules have been build for electron. Usually at startup, there's a message to the effect that some module did not self-register. When I was in that state and tried to run this script, I just got repeated logs about inability to analyze the performance. If there's some way to detect that error and provide a more helpful message, that might be useful.
4ff4f95
to
60f5cfd
Compare
Thanks, @colin-grant-work . Good points, all. I've pushed commit 60f5cfd that should address them, including adding a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest changes look good to me and the error output provided useful feedback where possible.
60f5cfd
to
b682bdc
Compare
The force-push of commit edc22d5 just resolves the conflict in the change log. |
b682bdc
to
edc22d5
Compare
@cdamus, please rebase this branch, and then I will merge it. |
- refactor a reusable module out of the startup measurement script - create a new script to measure Electron start-up performance - rename the measure-performance.js script to indicate that it measures the browser app specifically - add an --app/-a option to the extension-impact script to drive the Electron app as an alternative to the Browser app. Includes a bit of refactoring of this script and the app package template - incidentally fix the extension-impact.js stepping over itself trying to run the app while building it (was missing an await) Contributed on behalf of STMicroelectronics. Co-authored-by: Stefan Dirix <[email protected]> Signed-off-by: Christian W. Damus <[email protected]>
edc22d5
to
5242fbe
Compare
Thanks, @colin-grant-work I've force-pushed 5242fbe. The only conflict was in the changelog. |
What it does
Adds a new performance script for measurement of frontend start-up performance of the Electron example, similar to what was done in PR #9777. In fact, much of that script is factored out into a module of reusable functions to build the Electron script. This new script runs in much the same way, with similar command-line arguments to specify the number of runs and where to put the Chromium trace files.
Resolves #10440.
Example output:
Note that the existing NPM script for the browser start-up performance is renamed from
performance:startup
toperformance:startup:browser
to distinguish it from the Electron case.A new command-line option
--app
(-a
) is added to theextension-impact.js
script that selects the example application to build for execution of the tests. Supported values arebrowser
andelectron
; the default if not specified is the browser app as before.No new package dependencies are added.
Contributed on behalf of STMicroelectronics.
How to test
See the readme in the
scripts/performance/
directory for details of how to run the script. For most cases, it is sufficient just to run the NPM script from the monorepo root:$ yarn performance:startup:electron
This will print out performance measurements with summary statistics as shown above. You may also import the traces generated by the script in the
scripts/performance/profiles/electron/
directory into the Performance tab of the Chrome Developer Tools for analysis.Also, try running the Extension Impact script with the new option to drive the Electron app:
Note that executing the extension impact script on the Electron app after having run it on the browser app will often fail to get measurements for the
@theia/git
extension; it throws an exception about a mismatch in node versions in the native modules:This can be remedied by cleaning the Git workspace and building it again before running the script.
Review checklist
Reminder for reviewers