-
Notifications
You must be signed in to change notification settings - Fork 584
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 milestone and measurement updates for minecraft #10309
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -38,20 +38,50 @@ namespace pxt { | |||||
let eventLogger: TelemetryQueue<string, Map<string>, Map<number>>; | ||||||
let exceptionLogger: TelemetryQueue<any, string, Map<string>>; | ||||||
|
||||||
type EventListener<T = any> = (payload: T) => void; | ||||||
type EventSource<T> = { | ||||||
subscribe(listener: (payload: T) => void): () => void; | ||||||
emit(payload: T): void; | ||||||
}; | ||||||
|
||||||
function createEventSource<T = any>(): EventSource<T> { | ||||||
const listeners: EventListener<T>[] = []; | ||||||
|
||||||
return { | ||||||
subscribe(listener: EventListener<T>): () => void { | ||||||
listeners.push(listener); | ||||||
// Return an unsubscribe function | ||||||
return () => { | ||||||
const index = listeners.indexOf(listener); | ||||||
if (index !== -1) { | ||||||
listeners.splice(index, 1); | ||||||
} | ||||||
}; | ||||||
}, | ||||||
emit(payload: T): void { | ||||||
listeners.forEach(listener => listener(payload)); | ||||||
} | ||||||
}; | ||||||
} | ||||||
|
||||||
// performance measuring, added here because this is amongst the first (typescript) code ever executed | ||||||
export namespace perf { | ||||||
let enabled: boolean; | ||||||
|
||||||
export const onMilestone = createEventSource<{ milestone: string, time: number, params?: Map<string> }>(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason we wouldn't want a type for event sources? I know these two objects have differences, but is there a foundation we would want to have for event sources? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They do have a type, it's the type returned by |
||||||
export const onMeasurement = createEventSource<{ name: string, start: number, duration: number, params?: Map<string> }>(); | ||||||
|
||||||
export let startTimeMs: number; | ||||||
export let stats: { | ||||||
// name, start, duration | ||||||
durations: [string, number, number][], | ||||||
// name, start, duration, params | ||||||
durations: [string, number, number, Map<string>?][], | ||||||
// name, event | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
milestones: [string, number][] | ||||||
milestones: [string, number, Map<string>?][] | ||||||
} = { | ||||||
durations: [], | ||||||
milestones: [] | ||||||
} | ||||||
export function isEnabled() { return enabled; } | ||||||
export let perfReportLogged = false | ||||||
export function splitMs(): number { | ||||||
return Math.round(performance.now() - startTimeMs) | ||||||
|
@@ -75,8 +105,10 @@ namespace pxt { | |||||
return prettyStr(splitMs()) | ||||||
} | ||||||
|
||||||
export function recordMilestone(msg: string, time: number = splitMs()) { | ||||||
stats.milestones.push([msg, time]) | ||||||
export function recordMilestone(msg: string, params?: Map<string>) { | ||||||
const time = splitMs() | ||||||
stats.milestones.push([msg, time, params]) | ||||||
onMilestone.emit({ milestone: msg, time, params }); | ||||||
} | ||||||
export function init() { | ||||||
enabled = performance && !!performance.mark && !!performance.measure; | ||||||
|
@@ -89,7 +121,7 @@ namespace pxt { | |||||
export function measureStart(name: string) { | ||||||
if (enabled) performance.mark(`${name} start`) | ||||||
} | ||||||
export function measureEnd(name: string) { | ||||||
export function measureEnd(name: string, params?: Map<string>) { | ||||||
if (enabled && performance.getEntriesByName(`${name} start`).length) { | ||||||
performance.mark(`${name} end`) | ||||||
performance.measure(`${name} elapsed`, `${name} start`, `${name} end`) | ||||||
|
@@ -98,7 +130,8 @@ namespace pxt { | |||||
let measure = e[0] | ||||||
let durMs = measure.duration | ||||||
if (durMs > 10) { | ||||||
stats.durations.push([name, measure.startTime, durMs]) | ||||||
stats.durations.push([name, measure.startTime, durMs, params]) | ||||||
onMeasurement.emit({ name, start: measure.startTime, duration: durMs, params }); | ||||||
} | ||||||
} | ||||||
performance.clearMarks(`${name} start`) | ||||||
|
@@ -108,34 +141,44 @@ namespace pxt { | |||||
} | ||||||
export function report(filter: string = null) { | ||||||
perfReportLogged = true; | ||||||
|
||||||
if (enabled) { | ||||||
const milestones: {[index: string]: number} = {}; | ||||||
const durations: {[index: string]: number} = {}; | ||||||
const milestones: { [index: string]: number } = {}; | ||||||
const durations: { [index: string]: number } = {}; | ||||||
|
||||||
let report = `performance report:\n` | ||||||
for (let [msg, time] of stats.milestones) { | ||||||
let report = `Performance Report:\n` | ||||||
report += `\n` | ||||||
report += `\tMilestones:\n` | ||||||
for (let [msg, time, params] of stats.milestones) { | ||||||
if (!filter || msg.indexOf(filter) >= 0) { | ||||||
let pretty = prettyStr(time) | ||||||
report += `\t\t${msg} @ ${pretty}\n` | ||||||
report += `\t\t${msg} @ ${pretty}` | ||||||
for (let k of Object.keys(params || {})) { | ||||||
report += `\n\t\t\t${k}: ${params[k]}` | ||||||
} | ||||||
report += `\n` | ||||||
milestones[msg] = time; | ||||||
} | ||||||
} | ||||||
|
||||||
report += `\n` | ||||||
for (let [msg, start, duration] of stats.durations) { | ||||||
report += `\tMeasurements:\n` | ||||||
for (let [msg, start, duration, params] of stats.durations) { | ||||||
let filterIncl = filter && msg.indexOf(filter) >= 0 | ||||||
if ((duration > 50 && !filter) || filterIncl) { | ||||||
let pretty = prettyStr(duration) | ||||||
report += `\t\t${msg} took ~ ${pretty}` | ||||||
if (duration > 1000) { | ||||||
report += ` (${prettyStr(start)} - ${prettyStr(start + duration)})` | ||||||
for (let k of Object.keys(params || {})) { | ||||||
report += `\n\t\t\t${k}: ${params[k]}` | ||||||
} | ||||||
} | ||||||
report += `\n` | ||||||
} | ||||||
durations[msg] = duration; | ||||||
} | ||||||
console.log(report) | ||||||
enabled = false; // stop collecting milestones and measurements after report | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In doing a quick skim, it looks like enabled is otherwise only getting set at the initialization of the perf loading. If this is the case, will it be possible to grab more reports after the webapp first loading? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made this change because I believe it is undesirable to continue collecting perf measurements after the perf report is generated. These measurements accumulate in memory over time and never flush, which is bad if left unbounded (which it was). These metrics are most useful for gauging startup performance, so I think we don't want to continue collecting them once the editor is fully loaded. Let me know if I'm mistaken! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understand correctly, these are pretty generic methods. I worry someone might want to use them down the line (to measure issues with perf when hitting the play button, for example) and get confused as to why they're not working. Maybe we could have a flag to control this behavior? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @thsparks I agree it could be useful beyond startup. Supporting this would require coming up with a flushing strategy beyond what exists today. That's out of scope of this change. |
||||||
return { milestones, durations }; | ||||||
} | ||||||
return undefined; | ||||||
|
@@ -210,7 +253,7 @@ namespace pxt { | |||||
|
||||||
// App Insights automatically sends a page view event on setup, but we send our own later with additional properties. | ||||||
// This stops the automatic event from firing, so we don't end up with duplicate page view events. | ||||||
if(envelope.baseType == "PageviewData" && !envelope.baseData.properties) { | ||||||
if (envelope.baseType == "PageviewData" && !envelope.baseData.properties) { | ||||||
return false; | ||||||
} | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
namespace Measurements { | ||
export const TutorialInfoDbSetAsync = "tutorial info db setAsync"; | ||
export const ReloadAppTargetVariant = "reloadAppTargetVariant"; | ||
export const Sha256Buffer = "sha256buffer"; | ||
export const WebworkerRecvHandler = "webworker recvHandler"; | ||
export const NetworkRequest = "network.request"; | ||
} |
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.
What's the purpose of having a generic here if we're setting it to
any
?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.
any
is just a default type, effectively making the type optional. Some event sources might not want to pass a parameter. I do provide an explicit type in the instances below (viacreateEventSource
).