-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Make perfLogger potentially undefined rather than using a noop logger #53229
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
Make perfLogger potentially undefined rather than using a noop logger #53229
Conversation
@typescript-bot perf test this |
Heya @jakebailey, I've started to run the perf test suite on this PR at 99e167c. You can monitor the build here. Update: The results are in! |
@jakebailey Here they are:
CompilerComparison Report - main..53229
System
Hosts
Scenarios
TSServerComparison Report - main..53229
System
Hosts
Scenarios
StartupComparison Report - main..53229
System
Hosts
Scenarios
Developer Information: |
Seems like nearly no perf change (which I kind of figured anyway since the JIT will quickly inline noops out of existence). That said, |
Yes, I'd want this to be merged regardless of perf. |
I'm almost surprised this didn't hurt perf, since it theoretically adds branching costs |
The runtime is surprisingly good at figuring out the downleveled |
This use to be big perf issue where if you did undefined check before calling function it would be slower compared to noop function. Looks like its not anymore ? @rbuckton might know more if thats true as i havent followed changes to this from runtime perspective to know if it impacts perf or not in recent runtimes. |
We did this for tracing back in #42323, so, if there were any perf gotchas, I think they are all solved at this point. |
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 change looks OK to me. As long as there's no performance regression, I don't have strong opinions about it.
Effectively 100% of the time, ETW has not been loaded. Rather than using a noopLogger object, instead make
perfLogger
undefined when ETW isn't loaded, and reference with?.
just like our other tracing API. This should be faster, and hopefully observable since we trace some of our most-called APIs like file reads/writes, parsing/binding, module resolution, etc.