-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Do not calculate signatures if old state is not used #43314
Conversation
…l intent of test is maintained
…file version as signature
@@ -951,7 +951,7 @@ function bar() { | |||
} | |||
|
|||
// Change file1 get affected file list | |||
verifyLocalEdit(file1, "hello", "world"); | |||
verifyLocalEdit(file1, "hello", "world", /*returnsAllFilesAsAffected*/ !declaration); // Signatures are not initialized before this 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.
I wonder if compileOnSave should disable this behavior given it doesnt save all files so will be anyways calculating signatures for affected set and not going to change from one edit to next ?
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.
@uniqueiniquity @jessetrinity @amcasey thoughts ?
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.
@sheetalkamat Can you please elaborate? I think you're saying
- Old behavior: compute (emit?) .d.ts on compile-on-save
- New behavior: don't compute (emit?) .d.ts on (first?) compile-on-save
- Proposed behavior: restore old behavior
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.
If @amcasey's explanation is correct, then yes - my instinct says that compile-on-save should compute+emit .d.ts.
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.
Yes thats what i meant.. i will revert to old behavior for compile on save
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.
Updated and ready for review
Does this make the first watch build as fast as a non-watch build or is there still some overhead? |
yes, it should be about as fast. There is probably still an overhead, but it's unnoticeable. |
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 some basic benchmarking and it works as expected for the usual cases.
The following unusual case is still slow: Broken config file, compile, fix config file, compile. |
…on as signature in when state is not reused
23e6625
to
44ba0ec
Compare
Changes in this:
925e70e Just updates tests to ensure we are testing the scenarios we intended.
dd1cef2 is actual change and may be ideal to look at the changes as part of that commit.
69ebfe3 Checks updates to the incremental correctness of the program, essentially signature is same as d.ts emit signature or version of the file. Also exported modules map is checked in similar way.
44ba0ec Reverts the compileOnSave to old behavior of always computing signature
This is simplified implementation of work in #42960 by @sokra
There are more todos that we can improve on, eg if global file is changed, mark it for lazy signatures etc but i think that each change should be separate change to be able to evaluate the perf impact and if needed revert it.
Potential improvements: