Skip to content

avoid computing a full shape for indirectly invalidated files #44090

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

Merged
merged 2 commits into from
Jun 22, 2021

Conversation

sokra
Copy link
Contributor

@sokra sokra commented May 14, 2021

This fixes a worse case performance scenarios when a file has been changed that indirectly affects a lot of other files.

In such cases handleDtsMayChangeOf will be called for all indirectly affected files and will call updateShapeSignature to keep the build info valid. This means a lot files will have their shape computed and we already know that this is expensive.

This PR will switch all indirectly affected files to useFileVersionAsSignature mode, which is cheaper and the signature doesn't affect this update anyway. It will indeed affect future updates to these files, as not having a up-to-date signature will always invalidate referencing files.

To test this I added a list of files that have their shape computed (and in which way) to the baselines (first commit). Please make sure to review each commit on its own to avoid a lot of noise.

Open Questions

  • Should we delay computing the shapes by handleDtsMayChangeOf to a point after all affected files have been processed?
    This would make sure that these files would use full signature instead of signature as file version
    We could keep a list of these files and process them when getNextAffectedFile is Done
  • Should we honor disableUseFileVersionAsSignature for this too?

cc @sheetalkamat

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label May 14, 2021
@typescript-bot
Copy link
Collaborator

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

@sheetalkamat sheetalkamat self-assigned this May 14, 2021
@sheetalkamat
Copy link
Member

Thank you @sokra for investigating and coming up with this. We will soon look into this.

Note to self todo Next week:

  • Verify what this approach means for overall strategy, is it explainable when user reports issue and what is expected?
  • Open questions in the PR description based on 1

@sokra
Copy link
Contributor Author

sokra commented May 15, 2021

Note that using file version as signature has also some destructive effect to exportedModules, which makes future updates affect more files.
But I think it doesn't have to be this way. I think if the file is not directly affected, it's not possible that exportedModules can change. So we could continue using the exportedModules computed before. Is this assumption correct? If yes, and if we move this computation after handling all affected files, we could keep the exportedModules in updateShapeSignature from handleDtsMayChangeOf

@sheetalkamat sheetalkamat requested a review from amcasey June 8, 2021 02:10
@sheetalkamat
Copy link
Member

@amcasey @andrewbranch
can you please look into reviewing this.
With #44090 (comment) i made note of what all i want to verify.
The main concern is loosing the exports map and keeping short set of files to update if signature is already computed so you will have to verify the implications on perf, user perceptability of this change (eg how to diagnose the issues as well as communicate to users about the change)
Also need to verify that compileOnSave behaviour doesnt change with this change. (Test if missing needs to be added)

@sheetalkamat sheetalkamat requested a review from andrewbranch June 8, 2021 02:13
@amcasey
Copy link
Member

amcasey commented Jun 18, 2021

@sokra Thanks for your patience! I think we've concluded that the best way to validate this is to get it into the 4.4 beta. Can you please address the conflicts so we can merge the PR?

sokra added 2 commits June 22, 2021 11:04
using file version as shape is enough to keep build info valid
and it's much cheaper
@sokra sokra force-pushed the perf/shape-computation branch from daea8a2 to 8e20a9a Compare June 22, 2021 09:11
@sokra
Copy link
Contributor Author

sokra commented Jun 22, 2021

@amcasey rebased and updated baselines for some newly added test cases.

@amcasey amcasey merged commit 22637a2 into microsoft:main Jun 22, 2021
@sokra sokra deleted the perf/shape-computation branch June 23, 2021 08:43
@sokra
Copy link
Contributor Author

sokra commented Jun 23, 2021

also see #44690 for a little performance fix I found during implementing this pr...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants