Skip to content

Commit a44d96f

Browse files
weswighamweswigham
authored
Make CheckerPool iteration concurrent by default (#2070)
1 parent beeea27 commit a44d96f

File tree

4 files changed

+77
-66
lines changed

4 files changed

+77
-66
lines changed

‎internal/compiler/checkerpool.go‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,11 @@ import (
1212
)
1313

1414
type CheckerPool interface {
15+
Count() int
1516
GetChecker(ctx context.Context) (*checker.Checker, func())
1617
GetCheckerForFile(ctx context.Context, file *ast.SourceFile) (*checker.Checker, func())
1718
GetCheckerForFileExclusive(ctx context.Context, file *ast.SourceFile) (*checker.Checker, func())
18-
GetAllCheckers(ctx context.Context) ([]*checker.Checker, func())
19+
ForEachCheckerParallel(ctx context.Context, cb func(idx int, c *checker.Checker))
1920
Files(checker *checker.Checker) iter.Seq[*ast.SourceFile]
2021
}
2122

@@ -42,6 +43,10 @@ func newCheckerPool(checkerCount int, program *Program) *checkerPool {
4243
return pool
4344
}
4445

46+
func (p *checkerPool) Count() int {
47+
return p.checkerCount
48+
}
49+
4550
func (p *checkerPool) GetCheckerForFile(ctx context.Context, file *ast.SourceFile) (*checker.Checker, func()) {
4651
p.createCheckers()
4752
checker := p.fileAssociations[file]
@@ -82,9 +87,19 @@ func (p *checkerPool) createCheckers() {
8287
})
8388
}
8489

85-
func (p *checkerPool) GetAllCheckers(ctx context.Context) ([]*checker.Checker, func()) {
90+
// Runs `cb` for each checker in the pool concurrently, locking and unlocking checker mutexes as it goes,
91+
// making it safe to call `ForEachCheckerParallel` from many threads simultaneously.
92+
func (p *checkerPool) ForEachCheckerParallel(ctx context.Context, cb func(idx int, c *checker.Checker)) {
8693
p.createCheckers()
87-
return p.checkers, noop
94+
wg := core.NewWorkGroup(p.program.SingleThreaded())
95+
for idx, checker := range p.checkers {
96+
wg.Queue(func() {
97+
p.locks[idx].Lock()
98+
defer p.locks[idx].Unlock()
99+
cb(idx, checker)
100+
})
101+
}
102+
wg.RunAndWait()
88103
}
89104

90105
func (p *checkerPool) Files(checker *checker.Checker) iter.Seq[*ast.SourceFile] {

‎internal/compiler/program.go‎

Lines changed: 43 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"slices"
99
"strings"
1010
"sync"
11+
"sync/atomic"
1112

1213
"github.com/go-json-experiment/json"
1314
"github.com/microsoft/typescript-go/internal/ast"
@@ -351,28 +352,22 @@ func (p *Program) BindSourceFiles() {
351352
}
352353

353354
func (p *Program) CheckSourceFiles(ctx context.Context, files []*ast.SourceFile) {
354-
wg := core.NewWorkGroup(p.SingleThreaded())
355-
checkers, done := p.checkerPool.GetAllCheckers(ctx)
356-
defer done()
357-
for _, checker := range checkers {
358-
wg.Queue(func() {
359-
for file := range p.checkerPool.Files(checker) {
360-
if files == nil || slices.Contains(files, file) {
361-
checker.CheckSourceFile(ctx, file)
362-
}
355+
p.checkerPool.ForEachCheckerParallel(ctx, func(_ int, checker *checker.Checker) {
356+
for file := range p.checkerPool.Files(checker) {
357+
if files == nil || slices.Contains(files, file) {
358+
checker.CheckSourceFile(ctx, file)
363359
}
364-
})
365-
}
366-
wg.RunAndWait()
360+
}
361+
})
367362
}
368363

369364
// Return the type checker associated with the program.
370365
func (p *Program) GetTypeChecker(ctx context.Context) (*checker.Checker, func()) {
371366
return p.checkerPool.GetChecker(ctx)
372367
}
373368

374-
func (p *Program) GetTypeCheckers(ctx context.Context) ([]*checker.Checker, func()) {
375-
return p.checkerPool.GetAllCheckers(ctx)
369+
func (p *Program) ForEachCheckerParallel(ctx context.Context, cb func(idx int, c *checker.Checker)) {
370+
p.checkerPool.ForEachCheckerParallel(ctx, cb)
376371
}
377372

378373
// Return a checker for the given file. We may have multiple checkers in concurrent scenarios and this
@@ -965,14 +960,12 @@ func (p *Program) GetGlobalDiagnostics(ctx context.Context) []*ast.Diagnostic {
965960
return nil
966961
}
967962

968-
var globalDiagnostics []*ast.Diagnostic
969-
checkers, done := p.checkerPool.GetAllCheckers(ctx)
970-
defer done()
971-
for _, checker := range checkers {
972-
globalDiagnostics = append(globalDiagnostics, checker.GetGlobalDiagnostics()...)
973-
}
963+
globalDiagnostics := make([][]*ast.Diagnostic, p.checkerPool.Count())
964+
p.checkerPool.ForEachCheckerParallel(ctx, func(idx int, checker *checker.Checker) {
965+
globalDiagnostics[idx] = checker.GetGlobalDiagnostics()
966+
})
974967

975-
return SortAndDeduplicateDiagnostics(globalDiagnostics)
968+
return SortAndDeduplicateDiagnostics(slices.Concat(globalDiagnostics...))
976969
}
977970

978971
func (p *Program) GetDeclarationDiagnostics(ctx context.Context, sourceFile *ast.SourceFile) []*ast.Diagnostic {
@@ -1033,22 +1026,23 @@ func (p *Program) getSemanticDiagnosticsForFileNotFilter(ctx context.Context, so
10331026
defer done()
10341027
}
10351028
diags := slices.Clip(sourceFile.BindDiagnostics())
1036-
checkers, closeCheckers := p.checkerPool.GetAllCheckers(ctx)
1037-
defer closeCheckers()
10381029

10391030
// Ask for diags from all checkers; checking one file may add diagnostics to other files.
10401031
// These are deduplicated later.
1041-
for _, checker := range checkers {
1032+
checkerDiags := make([][]*ast.Diagnostic, p.checkerPool.Count())
1033+
p.checkerPool.ForEachCheckerParallel(ctx, func(idx int, checker *checker.Checker) {
10421034
if sourceFile == nil || checker == fileChecker {
1043-
diags = append(diags, checker.GetDiagnostics(ctx, sourceFile)...)
1035+
checkerDiags[idx] = checker.GetDiagnostics(ctx, sourceFile)
10441036
} else {
1045-
diags = append(diags, checker.GetDiagnosticsWithoutCheck(sourceFile)...)
1037+
checkerDiags[idx] = checker.GetDiagnosticsWithoutCheck(sourceFile)
10461038
}
1047-
}
1039+
})
10481040
if ctx.Err() != nil {
10491041
return nil
10501042
}
10511043

1044+
diags = append(diags, slices.Concat(checkerDiags...)...)
1045+
10521046
// !!! This should be rewritten to work like getBindAndCheckDiagnosticsForFileNoCache.
10531047

10541048
isPlainJS := ast.IsPlainJSFile(sourceFile, compilerOptions.CheckJs)
@@ -1140,22 +1134,20 @@ func (p *Program) getSuggestionDiagnosticsForFile(ctx context.Context, sourceFil
11401134

11411135
diags := slices.Clip(sourceFile.BindSuggestionDiagnostics)
11421136

1143-
checkers, closeCheckers := p.checkerPool.GetAllCheckers(ctx)
1144-
defer closeCheckers()
1145-
1146-
// Ask for diags from all checkers; checking one file may add diagnostics to other files.
1147-
// These are deduplicated later.
1148-
for _, checker := range checkers {
1137+
checkerDiags := make([][]*ast.Diagnostic, p.checkerPool.Count())
1138+
p.checkerPool.ForEachCheckerParallel(ctx, func(idx int, checker *checker.Checker) {
11491139
if sourceFile == nil || checker == fileChecker {
1150-
diags = append(diags, checker.GetSuggestionDiagnostics(ctx, sourceFile)...)
1140+
checkerDiags[idx] = checker.GetSuggestionDiagnostics(ctx, sourceFile)
11511141
} else {
11521142
// !!! is there any case where suggestion diagnostics are produced in other checkers?
11531143
}
1154-
}
1144+
})
11551145
if ctx.Err() != nil {
11561146
return nil
11571147
}
11581148

1149+
diags = append(diags, slices.Concat(checkerDiags...)...)
1150+
11591151
return diags
11601152
}
11611153

@@ -1251,32 +1243,28 @@ func (p *Program) SymbolCount() int {
12511243
for _, file := range p.files {
12521244
count += file.SymbolCount
12531245
}
1254-
checkers, done := p.checkerPool.GetAllCheckers(context.Background())
1255-
defer done()
1256-
for _, checker := range checkers {
1257-
count += int(checker.SymbolCount)
1258-
}
1259-
return count
1246+
var val atomic.Uint32
1247+
val.Store(uint32(count))
1248+
p.checkerPool.ForEachCheckerParallel(context.Background(), func(idx int, c *checker.Checker) {
1249+
val.Add(c.SymbolCount)
1250+
})
1251+
return int(val.Load())
12601252
}
12611253

12621254
func (p *Program) TypeCount() int {
1263-
var count int
1264-
checkers, done := p.checkerPool.GetAllCheckers(context.Background())
1265-
defer done()
1266-
for _, checker := range checkers {
1267-
count += int(checker.TypeCount)
1268-
}
1269-
return count
1255+
var val atomic.Uint32
1256+
p.checkerPool.ForEachCheckerParallel(context.Background(), func(idx int, c *checker.Checker) {
1257+
val.Add(c.TypeCount)
1258+
})
1259+
return int(val.Load())
12701260
}
12711261

12721262
func (p *Program) InstantiationCount() int {
1273-
var count int
1274-
checkers, done := p.checkerPool.GetAllCheckers(context.Background())
1275-
defer done()
1276-
for _, checker := range checkers {
1277-
count += int(checker.TotalInstantiationCount)
1278-
}
1279-
return count
1263+
var val atomic.Uint32
1264+
p.checkerPool.ForEachCheckerParallel(context.Background(), func(idx int, c *checker.Checker) {
1265+
val.Add(c.TotalInstantiationCount)
1266+
})
1267+
return int(val.Load())
12801268
}
12811269

12821270
func (p *Program) Program() *Program {

‎internal/project/checkerpool.go‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -94,22 +94,32 @@ func (p *CheckerPool) Files(checker *checker.Checker) iter.Seq[*ast.SourceFile]
9494
panic("unimplemented")
9595
}
9696

97-
func (p *CheckerPool) GetAllCheckers(ctx context.Context) ([]*checker.Checker, func()) {
97+
func (p *CheckerPool) Count() int {
98+
return p.maxCheckers
99+
}
100+
101+
func (p *CheckerPool) ForEachCheckerParallel(ctx context.Context, cb func(idx int, c *checker.Checker)) {
98102
p.mu.Lock()
99103
defer p.mu.Unlock()
100104

101105
requestID := core.GetRequestID(ctx)
102106
if requestID == "" {
103-
panic("cannot call GetAllCheckers on a project.checkerPool without a request ID")
107+
panic("cannot call ForEachCheckerParallel on a project.checkerPool without a request ID")
104108
}
105109

106110
// A request can only access one checker
107111
if c, release := p.getRequestCheckerLocked(requestID); c != nil {
108-
return []*checker.Checker{c}, release
112+
defer release()
113+
cb(0, c)
114+
return
109115
}
110116

117+
// TODO: Does this ever work without deadlocking? `p.GetChecker` also tries to lock this mutex.
118+
// Should this just be a panic?
111119
c, release := p.GetChecker(ctx)
112-
return []*checker.Checker{c}, release
120+
defer release()
121+
cb(0, c)
122+
return
113123
}
114124

115125
func (p *CheckerPool) getCheckerLocked(requestID string) (*checker.Checker, int) {

‎internal/testrunner/compiler_runner.go‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -529,9 +529,7 @@ func createHarnessTestFile(unit *testUnit, currentDirectory string) *harnessutil
529529
func (c *compilerTest) verifyUnionOrdering(t *testing.T) {
530530
t.Run("union ordering", func(t *testing.T) {
531531
p := c.result.Program.Program()
532-
checkers, done := p.GetTypeCheckers(t.Context())
533-
defer done()
534-
for _, c := range checkers {
532+
p.ForEachCheckerParallel(t.Context(), func(_ int, c *checker.Checker) {
535533
for union := range c.UnionTypes() {
536534
types := union.Types()
537535

@@ -549,7 +547,7 @@ func (c *compilerTest) verifyUnionOrdering(t *testing.T) {
549547
assert.Assert(t, slices.Equal(shuffled, types), "compareTypes does not sort union types consistently")
550548
}
551549
}
552-
}
550+
})
553551
})
554552
}
555553

0 commit comments

Comments
 (0)