Skip to content
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

New flag assumeChangesAffectShape for incremental and watch scenario #41219

Closed
wants to merge 3 commits into from

Conversation

sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented Oct 23, 2020

We normally use d.ts emit for a file as its signature so we can only emit or refresh diagnostics for only files that depend on it when shape of the file changes and any local change that doesn't affect its d.ts file, doesn't affect any other files in the compilation.
While this is good, there have been reports when people have run into issue with memory or perf because d.ts is slow.

Technical work around is to figure out where this slowness is coming from and explicitly annotate types so compiler doesn't have to manufacture them. But another possible work around is to use this new flag assumeChangesAffectShape which mean any change to file is assumed to change its shape.

The one side effect of this flag is that any change results in writing/analyzing more files than might be necessary but still could be acceptable. But this might be acceptable to people who run into issues with d.ts emit because they want to use --watch but arent looking to generate the d.ts files. As the compilation with local change might be slower than without this flag but could still be faster than running full build.

@DanielRosenwasser @RyanCavanaugh please consider if this is reasonable flag .. if yes, i can add reviewers to the change.
Possibly will help: #35729, #34916, #34119, #30732, #22826, #21140, #42937

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Oct 23, 2020
@sandersn sandersn added Experiment A fork with an experimental idea which might not make it into master and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Nov 2, 2020
@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Nov 2, 2020
@typescript-bot
Copy link
Collaborator

The PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@DanielRosenwasser
Copy link
Member

@sheetalkamat with this flag, would we have to recheck all dependencies (unless assumeChangesOnlyAffectDirectDependencies is on)?

@sheetalkamat
Copy link
Member Author

Yes this flag means, we will assume every change is "non local" unless assumeChangesOnlyAffectDirectDependencies is specified. But you are not spending time calculating d.ts so if someone has code that has more expensive d.ts calculation this is workaround for them.

@DanielRosenwasser
Copy link
Member

I wonder if we could consider a sort of combined approach. For example, given a module:

  • figure out the set of dependent modules. If the set of dependencies is "large", then use the .d.ts output as the shape; otherwise, use the .js output
  • if a module gets changed often enough and it's using .js to determine shape, we switch to using its .d.ts output to determine shape

@sheetalkamat
Copy link
Member Author

I feel like those are some of the things we could do generally and shouldn't be affected by this flag.. this flag is escape route to users whose d.ts is not easily computable and they cant find the way to fix the original issue or waiting on us (which most of the time is design limitation, because type is complicated)

@sokra had done some changes on line of based on set delay d.ts computation as part of #42960 and i did mark those as TODO in #43314 which i plan to pick up in 4.4 individually so we its easier to measure, tweak, revert...

@DanielRosenwasser
Copy link
Member

Yeah, I was thinking about how some of the ideas there, here and stuff we've seen in GCs/JITs where hot paths get "warmed up" could kind of be explored. If you think might be promising, I can create an issue to track it.

@sheetalkamat
Copy link
Member Author

Definitely worth investigating

@sheetalkamat
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 5, 2021

Heya @sheetalkamat, I've started to run the tarball bundle task on this PR at d52da06. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 5, 2021

Hey @sheetalkamat, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/102296/artifacts?artifactName=tgz&fileId=6D6FE962F38015FFF593DC4B71D97CC830EA754EC6F42CB0320C719CF0C6556C02&fileName=/typescript-4.3.0-insiders.20210505.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@4.3.0-pr-41219-9".;

@sheetalkamat
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 7, 2021

Heya @sheetalkamat, I've started to run the tarball bundle task on this PR at 4028fbe. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 7, 2021

Hey @sheetalkamat, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/102590/artifacts?artifactName=tgz&fileId=D7BBB9D473C3D4F8A0E540B0099B4293F6AFFCD1B5D055B2F186D93C109110F402&fileName=/typescript-4.3.0-insiders.20210507.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@4.3.0-pr-41219-12".;

@sheetalkamat sheetalkamat deleted the assumeChangesAffectShape branch November 9, 2022 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team Experiment A fork with an experimental idea which might not make it into master For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants