updateRelatedChangeType perf improvements#1046
Merged
ecraig12345 merged 2 commits intomainfrom Apr 1, 2025
Merged
Conversation
derrickstolee
approved these changes
Apr 1, 2025
Collaborator
derrickstolee
left a comment
There was a problem hiding this comment.
Thanks for looking into performance improvements here!
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
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.
@derrickstolee's PR #1042 got me looking more closely at
updateRelatedChangeTypeand its performance, which eventually made me notice that I'd accidentally introduced a (sometimes) massive amount of duplicate processing in that function in #606...The problem is here: this function was always meant to process a single change info, but for some reason I updated it to process all changes from the same change file, which would then be repeated when the
bumpInPlaceloop runs for every other change from that file. This becomes a problem when a grouped change file has 3000 changes, and of course the graph traversal step is also running on that large graph...The main change in this PR is removing the outer loop from
updateRelatedChangeType, and updating it to bail out early if the change type is'none'. (I also noticed a lot of other historical weirdness and bugs, but I'm just focusing on lower-risk fixes for now.)Other smaller (non-test) changes:
readChangeFilesalready does that)initializePackageChangeTypesto ignore'none'changesgetMaxChangeType, reduce the number of cases wheregetAllowedChangeTypeis called fordisallowedChangeTypes