-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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 measure tracing API #9637
Add measure tracing API #9637
Conversation
9f8c706
to
125b37b
Compare
return await tracer.measureAsync( | ||
{ | ||
name, | ||
args: {bundle: bundle.name}, |
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.
This also used to track bundle type as well. Did you intend to drop that?
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.
Nope, will add it back in. Thanks for picking that up
125b37b
to
ae8d245
Compare
ae8d245
to
b375c28
Compare
b375c28
to
65ac565
Compare
408a1c8
to
df99038
Compare
|
||
createMeasurement( |
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 original reasoning for having an API that didn't involve passing an object was to avoid the overhead of unnecessary object allocation in the case when tracing was disabled (which is most of the time).
Have you checked that the impact of performance on builds with tracing disabled of this change? Does timing/memory usage look okay? I just want to make sure we're not introducing unnecessary GC thrashing that might affect builds the size of Jira's.
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 created a crude benchmark that can be run with node --trace-gc --trace-gc_verbose test.mjs
with this, commenting in one test at a time. I will revert the existing change so that it uses something more performant with more flexibility.
let enabled = false;
class Measurement {
constructor(args) {
this.args = args;
this.start = performance.now();
}
end() {
const end = performance.now();
// tracer.trace();
}
}
async function measure(args, fn) {
if (!enabled) {
return fn();
}
const measurement = new Measurement(args);
let result;
try {
result = await fn();
} finally {
measurement.end();
}
return result;
}
for (let i = 0; i < 100_000_000; i++) {
// fastest -> slowest
await measure('world', async () => {});
// await measure(enabled && { hello: 'world' }, async () => {});
// await measure(enabled ? { hello: 'world' } : null, async () => {});
// await measure({ hello: 'world' }, async () => {});
// await measure(() => ({ hello: 'world' }), async () => {});
// await measure({ get hello() { return 'world' }}, async () => {});
}
// for (let i = 0; i < 1_000_000_000; i++) {
// measure({ hello: 'world' }, () => {});
// measure('world', () => {});
// }
848bebd
to
b7c4911
Compare
type: bundle.type, | ||
}); | ||
measurement = | ||
tracer.enabled && |
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 know moving this check to the consumer avoids the function call at all, but is the only reason to change the create
signature from individual params to an object? I'm still not sure what the benefit is apart from the naming?
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 the naming and flexibility mostly, yeah
↪️ Pull Request
This change is similar to #9622 but instead introduces non-breaking changes that:
createTraceMeasurement
API, by only instantiating relevant event data when tracing is enabled at the consumer sidetraceStart
eventAdditionally, some arguments have been updated to be more meaningful.
💻 Examples
N/A
🚨 Test instructions
Run with tracing enabled