Skip to content

chore: make fiximports run in parallel#12985

Merged
rvagg merged 3 commits intomasterfrom
rvagg/fiximports-parallel
Mar 28, 2025
Merged

chore: make fiximports run in parallel#12985
rvagg merged 3 commits intomasterfrom
rvagg/fiximports-parallel

Conversation

@rvagg
Copy link
Copy Markdown
Member

@rvagg rvagg commented Mar 27, 2025

fiximports has been driving me a little insane lately. For some reason it's started taking up to ~40s each execution and I can't figure out why. I tried switched from running go from a snap to just installing it from a package, I tried clearing my go build cache, I tried removing all the x/tools sources I had. Maybe I made a difference somewhere in there, but I'm still ~20s.

When measuring, the time cost is all in imports.Process, so something in there isn't having a happy time.

After a couple of iterations of trying to fix it I've landed on the following approach which gives me the best results:

  1. Find all of the files we need to process
  2. Read all file contents in (+ do the quick imports newline squash)
  3. For each mutation of imports.LocalPrefix, process all files in parallel—so we still do imports.Process twice but we do the files in parallel and don't need to worry about imports.LocalPrefix mutation
  4. Write out any files that have been changed

Before:

$ time make fiximports 
go run ./scripts/fiximports

real    0m20.004s
user    0m15.952s
sys     0m12.062s

After:

$ time make fiximports 
go run ./scripts/fiximports

real    0m1.859s
user    0m16.487s
sys     0m16.172s

Almost too good to be true? Seems to work just fine though. @masih would you mind taking a good look at this when you have some time please?

@rvagg rvagg added the skip/changelog This change does not require CHANGELOG.md update label Mar 27, 2025
@rvagg rvagg requested a review from masih March 27, 2025 11:53
@github-project-automation github-project-automation Bot moved this to 📌 Triage in FilOz Mar 27, 2025
@rvagg rvagg requested a review from Copilot March 27, 2025 11:53
Copy link
Copy Markdown
Contributor

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 fiximports script to improve performance by processing files in parallel.

  • Introduces concurrent reading, processing, and writing of file contents.
  • Replaces the sequential fixGoImports function with parallel pipelines using worker routines.

Comment thread scripts/fiximports/main.go Outdated
@masih
Copy link
Copy Markdown
Member

masih commented Mar 27, 2025

Seen #11695 (comment) ?

@rvagg
Copy link
Copy Markdown
Member Author

rvagg commented Mar 27, 2025

No, but

I think the thing to do is to simplify our formatting to two groups of SDK and no SDK

Yes, that was my initial thought as I was working on this because it's that global that stops simple parallelism. Figured I'd try for least-change though and this yields pretty good results.

Copy link
Copy Markdown
Member

@masih masih left a comment

Choose a reason for hiding this comment

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

No blockers.

I think the logic can be simplified quite a bit using error groups.

Comment thread scripts/fiximports/main.go Outdated
Comment thread scripts/fiximports/main.go Outdated
Comment thread scripts/fiximports/main.go
@github-project-automation github-project-automation Bot moved this from 📌 Triage to ✔️ Approved by reviewer in FilOz Mar 27, 2025
@rvagg
Copy link
Copy Markdown
Member Author

rvagg commented Mar 28, 2025

Yeah, errgroups made it much better, thanks. PTAL @masih

@rvagg rvagg requested review from Copilot and masih March 28, 2025 00:51
Copy link
Copy Markdown
Contributor

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 addresses the performance issues in the fiximports tool by processing files in parallel. Key changes include:

  • Reading file contents concurrently before processing.
  • Using parallel execution to run imports.Process for multiple prefixes.
  • Writing modified files concurrently after processing.

Comment thread scripts/fiximports/main.go
Comment thread scripts/fiximports/main.go
Comment thread scripts/fiximports/main.go
@rvagg rvagg force-pushed the rvagg/fiximports-parallel branch from 7d93e11 to c9e5ef4 Compare March 28, 2025 00:52
@rvagg rvagg requested a review from Copilot March 28, 2025 00:56
Copy link
Copy Markdown
Contributor

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 aims to significantly reduce the execution time of fiximports by processing files in parallel, reducing the overall runtime drastically.

  • Converts file reading, processing, and writing to parallel operations.
  • Leverages errgroup with a worker limit based on the number of CPUs.

Comment thread scripts/fiximports/main.go
Comment thread scripts/fiximports/main.go
Comment thread scripts/fiximports/main.go
@rvagg rvagg enabled auto-merge (squash) March 28, 2025 11:45
@rvagg rvagg merged commit 6ba68ca into master Mar 28, 2025
90 checks passed
@rvagg rvagg deleted the rvagg/fiximports-parallel branch March 28, 2025 11:45
@github-project-automation github-project-automation Bot moved this from ✔️ Approved by reviewer to 🎉 Done in FilOz Mar 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip/changelog This change does not require CHANGELOG.md update

Projects

Status: 🎉 Done

Development

Successfully merging this pull request may close these issues.

3 participants