Skip to content

Conversation

sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented Aug 19, 2025

This should fix #1520 with unnecessary calculation and storage for "buildInfoProgram" where we dont even need referencedBy files

In the incremental builds:

Metric Before After Improvement
Memory allocations 149.7M 71.1M -52.51%
Memory usage 9.48MB 8.39MB -11.59%
BuildInfo read time 9.46s 0.95s -89.98%
Changes compute time 6.27s 3.79s -39.52%
Total time 48.13s 36.98s -23.17%

Fixes #1520
Fixes #1541

@Copilot Copilot AI review requested due to automatic review settings August 19, 2025 19:00
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the reference map implementation to compute referencedBy relationships only when needed, replacing the existing SyncManyToManySet with a custom referenceMap type that uses lazy initialization.

  • Introduces a new referenceMap type with lazy computation of reverse references
  • Replaces direct SyncManyToManySet usage with wrapper methods
  • Removes the now-unused SyncManyToManySet collection type

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
internal/incremental/referencemap.go New file implementing the referenceMap type with lazy referencedBy computation
internal/incremental/snapshot.go Changes field type from SyncManyToManySet to referenceMap
internal/incremental/snapshottobuildinfo.go Updates method calls to use new referenceMap API
internal/incremental/programtosnapshot.go Updates method calls to use new referenceMap API
internal/incremental/buildinfotosnapshot.go Updates method calls to use new referenceMap API
internal/incremental/affectedfileshandler.go Updates method calls and removes helper method that's no longer needed
internal/collections/syncmanytomanyset.go Removes the entire file as it's no longer used

@sheetalkamat sheetalkamat linked an issue Aug 19, 2025 that may be closed by this pull request
@sheetalkamat sheetalkamat enabled auto-merge August 20, 2025 01:20
@sheetalkamat sheetalkamat added this pull request to the merge queue Aug 20, 2025
Merged via the queue into main with commit 3a82d57 Aug 20, 2025
74 of 79 checks passed
@sheetalkamat sheetalkamat deleted the incrementalReferenceMap branch August 20, 2025 01:50
@joshcartme
Copy link
Contributor

This is huge and has made our cold and warm significantly faster. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

20% more memory use starting in 7.0.0-dev.20250724.1 --incremental builds are a bit slower after the 20250805 release
3 participants