-
Notifications
You must be signed in to change notification settings - Fork 704
Fixes to make tsc -b and incremental work for memory issues #1745
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
base: main
Are you sure you want to change the base?
Conversation
sheetalkamat
commented
Sep 22, 2025
- Buildinfo was on "toBuildInfo" object made it separate allocation so that it doesnt form cycle with incremental program and snapshot
- Do not store incremental program past compilation as we are storing buildInfo anyways
- handled single threaded build to build in order to ensure that it does not create a deadlock
- Handled the case where same file exists multiple times in program
- optimized to not calculate "referenceMap" and other incremental state when we are building for tsc -b for non incremental programs
…tal compilation but emitting buildInfo for tsc -b
There was a problem hiding this 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 implements memory optimizations for TypeScript incremental compilation and build functionality. The changes focus on reducing memory usage by breaking cycles between incremental programs and snapshots, avoiding unnecessary state tracking for non-incremental builds, and ensuring proper cleanup of resources.
Key changes include:
- Refactored buildInfo allocation to be separate from incremental program snapshots
- Optimized incremental state computation to only run when needed
- Restructured build task execution to support both single-threaded and multi-threaded modes
Reviewed Changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
internal/execute/incremental/snapshottobuildinfo.go | Separates buildInfo allocation and handles duplicate files in programs |
internal/execute/incremental/snapshot.go | Adds method to determine when incremental state tracking is needed |
internal/execute/incremental/programtosnapshot.go | Conditionally computes incremental state based on build requirements |
internal/execute/incremental/program.go | Removes MakeReadonly method and optimizes diagnostic collection |
internal/execute/incremental/emitfileshandler.go | Restructures emit handling to support both incremental and non-incremental paths |
internal/execute/build/orchestrator.go | Refactors build execution to handle single-threaded builds sequentially |
internal/execute/build/buildtask.go | Restructures task state management and reporting |
internal/execute/tsc/statistics.go | Updates statistics aggregation to work incrementally |
testdata/baselines/reference/* | Updates test baselines reflecting changes in incremental behavior |
if int(fileId) != len(t.buildInfo.FileNames) { | ||
// Duplicate - for now ignore | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The duplicate file handling logic should include a more descriptive comment explaining why duplicates occur and what the implications are of ignoring them. Consider adding logging or metrics to track when this condition is hit.
Copilot uses AI. Check for mistakes.
func emitFiles(ctx context.Context, program *Program, options compiler.EmitOptions, isForDtsErrors bool) *compiler.EmitResult { | ||
emitHandler := &emitFilesHandler{ctx: ctx, program: program, isForDtsErrors: isForDtsErrors} | ||
|
||
if options.TargetSourceFile != nil { | ||
if !isForDtsErrors && options.TargetSourceFile != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition combines two boolean checks that serve different purposes. Consider extracting this into a well-named helper method like shouldUseDirectEmit(isForDtsErrors, targetSourceFile)
to improve readability and make the intent clearer.
Copilot uses AI. Check for mistakes.
if o.opts.Command.CompilerOptions.SingleThreaded.IsTrue() { | ||
for _, config := range o.Order() { | ||
path := o.toPath(config) | ||
task := o.getTask(path) | ||
o.buildOrCleanProject(task, path, &buildResult) | ||
} | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The single-threaded execution path processes tasks in dependency order, while the multi-threaded path processes all tasks concurrently. Consider adding a comment explaining why the order matters for single-threaded builds but not for multi-threaded builds, as this could be a source of confusion.
Copilot uses AI. Check for mistakes.
FYI I tried this out (commit
![]() |