-
-
Notifications
You must be signed in to change notification settings - Fork 930
Implement profiler for lifecycle events. #868
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
Conversation
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.
Very minor comments.
src/lifecycles/bootstrap.js
Outdated
return Promise.resolve().then(() => { | ||
if (appOrParcel.status !== NOT_BOOTSTRAPPED) { | ||
return appOrParcel; | ||
} | ||
|
||
if (__PROFILE__) { | ||
startTime = Date.now(); |
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.
Do you want to consider using performance.now()
instead? https://developer.mozilla.org/en-US/docs/Web/API/Performance/now
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.
Nice, I didn't know about performance.now(). The one potential downside is you lose absolute timestamps (2021-09-14 00:00:00) in favor of timestamps that are relative to the time origin. But that might even be better, it's not clear that it's really a downside. So I'll switch them over.
export function addProfileEntry( | ||
type, | ||
name, | ||
kind, | ||
start, | ||
end, | ||
operationSucceeded | ||
) { |
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.
It's interesting that you take these in as separate params (which require remembering the order), but in the end store them as object properties in the end. Would it be easier to just make this an object from the get-go?
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.
I did them as separate params with the idea being it would keep bundle size down. It's true that i later put them in an object, but my thought was that at least the property names are only in one place rather than in every place that adds a profile entry?
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.
LGTM 🐽
This sets up the approach for profiling and then implements it for application and parcel lifecycles. To start with, I've just added the profiler to the single-spa dev build, not prod build. I know that React has a production profiler build but didn't think the performance difference between the dev and prod build to be large enough to justify adding a bunch of new bundles to the tarball published to npm.
I'm most interested in feedback on the profiler api and what data we track. My idea is that single-spa-inspector will call getProfilerData and then show some of the data in the browser extension and maybe even provide a way to download the profiler data to a CSV file or something.
Future PRs will add profiler events within the
reroute
function to track which parts of the rerouting takes longest.