-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Optimize external source maps without full cache #40130
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
Optimize external source maps without full cache #40130
Conversation
9b0caea
to
d5925cc
Compare
@typescript-bot test this |
Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at d5925cc. You can monitor the build here. |
Heya @DanielRosenwasser, I've started to run the parallelized community code test suite on this PR at d5925cc. You can monitor the build here. |
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at d5925cc. You can monitor the build here. Update: The results are in! |
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
@DanielRosenwasser Here they are:Comparison Report - master..40130
System
Hosts
Scenarios
|
d5925cc
to
729830a
Compare
Alright, those results look good to me. I'm not entirely sure if it is really faster or if it was variance, but where #40055 was mostly slower compared to master (I'm looking at "emit time"), this is mostly faster compared to master (which I can't really explain, hence the possibility that it was just variance). I rebased this on top of master, so it no longer contains the revert of #40055. Please let me know if there's anything else that needs to be done. Also, could this also go into a 4.0.x patch release as well? |
Yes this is improvement but i dont think this warrants the criteria for patch release.. So it should be targetted for 4.1 instead in my opinion. |
@sheetalkamat are you referring to the perf test results in #40130 (comment), or the table I posted in #40055 (comment)? The automated perf tests unfortunately won't show any improvements, as it's only external source map sources that take the slow path which is not something that Thanks for the quick turnarounds btw, hugely appreciated! |
Yes i meant the improvement for tooling. The reason for perf test was to ensure normal scenario does not regress. And even though this is imporvement(for tooling) this doesnt warrant patch release in my opinion. |
Alternative to #40055, which already landed but #40126 shows a minor regression, so this switches to a single MRU cache instead of a full cache.