-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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 a switch for reporting expensive statements #37785
Conversation
Marked as a draft because I guessed a bunch of things. |
let statistics: Statistic[]; | ||
const compilerOptions = program.getCompilerOptions(); | ||
|
||
if (compilerOptions.expensiveStatements) { |
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.
They're really only diagnostics for the pretty-printing. Otherwise, they seem like they belong with the statistics.
@@ -5733,5 +5737,9 @@ | |||
"An optional chain cannot contain private identifiers.": { | |||
"category": "Error", | |||
"code": 18030 | |||
}, | |||
"Checking this statement may result in the creation of as many as {0} types and {1} symbols.": { | |||
"category": "Warning", |
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 is the first Warning
. It feels pretty distinct to me, but it doesn't really matter one way or the other.
}, | ||
"Checking this statement may result in the creation of as many as {0} types and {1} symbols.": { | ||
"category": "Warning", | ||
"code": 19000 |
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.
Again, since this felt like a new kind of diagnostic, I added a new code range.
src/compiler/checker.ts
Outdated
checkSourceElementWorker(node); | ||
|
||
if (node.kind >= SyntaxKind.FirstStatement && node.kind <= SyntaxKind.LastStatement) { |
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 could be more aggressive about skipping work if the statements will never be reported.
src/compiler/checker.ts
Outdated
|
||
let i = 0; | ||
for (const record of expensiveStatements) { | ||
if (record.typeDelta < typeDelta) { |
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.
In teamroom discussions, there has seemed to be more interest in type count than symbol count, so I used that for sorting.
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 wonder if it'll be able to detect groups of statements that are cheap individually but are executed so often that they are actually expensive.
eg. I have some type ConvertNumbersToStringsInInterface<I>
that will be not so expensive as individual call.
But let's say I use it on ViewStyle
interface which can be included 100s of properties.
And then let's say I use it in every view I've got in my app. Then I can end up having 1000s of relatively cheap operations instead of 1 huge one.
Maybe it'll be possible to track the total time of each type being 'part of' for other operations?
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.
@pie6k I had an earlier version that listed top calls by both individual invocation and aggregate, but it proved to be too difficult to read the output. Anecdotally, this unsophisticated heuristic has been helpful in some real world projects, but, as you point out, it has numerous limitations.
@sheetalkamat Yes, I'll add a test if people are happy with the design. 😄 |
Hey, do you think it'd be possible/good idea to somehow group diagnostic report items? eg. Some projects might have relatively 'cheap' typing operation but used so many times that it might take most of the time spent processing types (it can be the case with packages like styled-components that are often used 100s of times in the codebase). In such cases, being able to detect that and extract as a group could be a good insight. Also, optimizing ts server for such cases even a little bit could make a big difference. Another question is if it's possible to run those diagnostics in 'ts server mode'? I mean running it for single operations in IDE (eg. requesting auto suggestions). Currently, I was able to run your diagnostics with In my case, being able to have diagnostics for single editor operations could be very helpful. Or maybe it could be possible to 'detect' long editor operations and then prepare diagnostic info for those (and send them to you guys?). In general, my priorities about 'performance' are way more focused on 'real-time' performance than on initial load/preparing ts-server for the first time. I'd be happy waiting twice as long for ts-server init and then having twice shorter real-time editor operations. @amcasey - well... if you want, I can give you access to the private repo I've got issues with. I'd like to avoid any leaks of it, but it's not rocket science or top-secret code. DM me on Twitter (@pie6k) if you'd like to set it up. Another thing is, that some of the issues I've got in my codebase are actually quite tricky to reproduce - ie they happen relatively often, but I have no idea what causes them. I don't want to run verbose logging all the time (as I think, but not sure, this is slowing everything even more) and makes logs so huge I have no idea what's going there at all. But if I have logs turned off and I have the issue - in order to collect logs I have to restart ts-server which wipes out the issue as it starts to work fine after ts-server restart. |
@typescript-bot pack this |
Ah, it's possible this can't be done because it's a draft PR |
@orta I don't think so - I routinely pack draft PRs |
@pie6k Sorry for the delay, I thought I'd already posted replies - I'd certainly thought through them. We're starting with batch/command line metrics because the scenarios are much more clear cut and reproducible. The problem with interactive scenarios is that a lot of operations are incremental and the exact order of operations makes it very tricky to identify or reproduce the causes of problems. Building up a profiling ecosystem for TypeScript will be a long journey and this is just the first step. Regarding your particular setup, if you're on Windows, it may be possible to keep very-inexpensive ETW logging running in the background that would give us nearly all the information that the text log would contain. Setup is a bit involved, but let me know if you're interested and I can write something up. |
@amcasey I'm using macOS. If something similar is possible - let me know, I'd be happy to do it, even if the setup is involved. |
@pie6k We've had trouble finding posix equivalents to ETW, but we're open to suggestions. DTrace seemed like the most obvious, but it didn't play nicely with our other infrastructure, so we've been sticking with text logs for now. |
Specific to Apple, it has a new and reasonably similar logging system - I took a look at trying to access it form a C++ cli app, and it relies on the objc runtime - which I originally thought was a blocker. Looks like @rebornix got that working in napi-spellchecker. So I think I could probably build this if we need it. |
@typescript-bot pack this |
@typescript-bot pack this |
@typescript-bot pack this |
I thought it was yesterday's GH instability, but it looks like the bot really can't pack this. I'll see if rebasing helps. |
f6ada20
to
d99f547
Compare
@typescript-bot pack this |
@typescript-bot pack this |
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at beb01a3. You can monitor the build here. |
beb01a3
to
2e794fe
Compare
@typescript-bot pack this |
Hey @amcasey, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build. |
@typescript-bot pack this |
@typescript-bot pack this |
Hey @amcasey, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build. |
...rather than just dropping the first one, now that .d.ts file checking is front-loaded.
Thanks to @RyanCavanaugh for the suggestion!
eabb93d
to
9d24de1
Compare
Superseded by #40063. |
Multiple customers have reported that having expensive statements identified (thus far, with private builds) has been helpful in reducing their check time - even without explanations of why the statements are expensive. This PR attempts to formalize that instrumentation so that it can be used more widely.