Avoid no-op export map updates #45238
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
...so that unchanged maps can be recognized without having to examine their contents.
In practice, `updateExportedFilesMapFromCache` is called repeatedly without the cache changing in between. When this occurs, there's no need to update the `BuilderState` (this was already the net effect, but it took a long time to determine that no work was required).
typescript-bot
added
Author: Team
For Uncommitted Bug
|
For my local repro 4.2: 631.23s |
|
@DanielRosenwasser @RyanCavanaugh I think it's important to get this into 4.4 because I don't have a good sense of whether/which real world scenarios will see the large build time regression I noticed in my (contrived) local experiments. |
amcasey
commented
Jul 29, 2021
amcasey
commented
Jul 29, 2021
| addToMultimap(reverse, v, k); | ||
| } | ||
| }); | ||
|
|
||
| if (changed) { | ||
| version++; |
There was a problem hiding this comment.
It's vitally important that this not miss a case where the map has actually changed - please double-check.
|
@typescript-bot perf test this |
|
Heya @amcasey, I've started to run the perf test suite on this PR at 03bfca3. You can monitor the build here. Update: The results are in! |
|
As far as I know, the perf suite doesn't cover build mode, so I don't expect any changes. Edit: confirmed |
|
@amcasey Here they are:Comparison Report - main..45238
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
DanielRosenwasser
approved these changes
Jul 29, 2021
There was a problem hiding this comment.
Seems fine to me, though there is some feedback you might want to consider. You should get a second look from @andrewbranch.
andrewbranch
approved these changes
Jul 29, 2021
sheetalkamat
added a commit
that referenced
this pull request
Apr 5, 2022
This reverts commit 0f6e6ef.
sheetalkamat
added a commit
that referenced
this pull request
Apr 6, 2022
This reverts commit 0f6e6ef.
sheetalkamat
added a commit
that referenced
this pull request
Apr 11, 2022
This reverts commit 0f6e6ef.
sheetalkamat
added a commit
that referenced
this pull request
Apr 13, 2022
This reverts commit 0f6e6ef.
sheetalkamat
added a commit
that referenced
this pull request
Apr 14, 2022
This reverts commit 0f6e6ef.
sheetalkamat
added a commit
that referenced
this pull request
Apr 14, 2022
This reverts commit 0f6e6ef.
Jack-Works
pushed a commit
to Jack-Works/TypeScript
that referenced
this pull request
Apr 22, 2022
…cache. (microsoft#48579) * Revert "Avoid no-op export map updates (microsoft#45238)" This reverts commit 0f6e6ef. * Need to reset currentAffectedFilesExportedModulesMap after commiting to final exports map
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a follow-up to #44402. Internal teams with large projects have seen significant perf wins from that change (~20% reduction in build time) but I've stumbled onto some pathological cases where the change actually increases build time by ~50%.
The builder maintains two copies of the exported modules map - changes are accumulated in a copy so that they can be discarded if something invalidated the update. When the build does complete successfully, one
ManyToManyPathMapis updated from another, which requires traversing both. In local measurements, each such update was taking 2-4 seconds (this is very project-specific) and there were 2000 in the course of building a single project. However, it turned out that the copy was never actually modified after its initialization, so all subsequent updates of the "real" map were no-ops. This change introduces a cheap way to determine that the cache hasn't changed.I think we could probably use a more sophisticated method to handle these updates (e.g. accumulating only a delta), but I deliberately made the change very simple because we're so late in the 4.4 cycle.