[tools] internal/imports: read the module index only where it is used
Brad Moylan (Gerrit)
Brad Moylan has uploaded the change for review![]( "Open in Gerrit")
Commit message
internal/imports: read the module index only where it is used
getFixesWithSource read the GOMODCACHE index on entry, but ix is not
used until the third pass, after three early returns. A file whose
imports are already complete returns from the first pass, having read
and parsed the entire index for nothing.
The cost is per file, not per run: the imports.Process wrapper builds a
fresh ProcessEnv for each call and modindex.Read is not memoized, so a
tool that formats a tree re-reads the index once per file. With a 60MB
index, goimports -l over 500 already-formatted files in x/tools took
41.5s and 233MB peak RSS; reading the index at its point of first use
instead brings that to 0.47s and 20MB.
Files that do need an import resolved still read the index and are
unaffected. Verified by comparing output before and after over 8220
files, and over a mutation corpus of 250 files in a non-vendored module
with one third-party import deleted from each, which exercises
addExternalCandidates; output is identical in every case.
This applies a change requested during review of CL 767881 (patch set
23) that was acknowledged but did not reach the submitted revision.
Fixes golang/go#80774
Change diff
diff --git a/internal/imports/fix.go b/internal/imports/fix.go
index 6aa1e27..274f635 100644
--- a/internal/imports/fix.go
+++ b/internal/imports/fix.go
@@ -579,17 +579,6 @@
}
func getFixesWithSource(ctx context.Context, fset *token.FileSet, f *ast.File, filename string, goroot string, logf func(string, ...any), source Source) ([]*ImportFix, error) {
- // If there is an Index for the GOMODCACHE, remember that, and later make it so that the
- // directory walk doesn't go into the module cache, since we already have all the information
- var ix *modindex.Index
- src, ok := source.(*ProcessEnvSource)
- if ok {
- var err error
- if ix, err = modindex.Read(src.env.Env["GOMODCACHE"]); err != nil {
- ix = nil // don't use it if there was an error
- }
- }
-
// This logic is defensively duplicated from getFixes.
abs, err := filepath.Abs(filename)
if err != nil {
@@ -648,6 +637,17 @@
}
p.loadRealPackageNames = true
p.otherFiles = otherFiles
+
+ // If there is an Index for the GOMODCACHE, remember that, and later make it so that the
+ // directory walk doesn't go into the module cache, since we already have all the information.
+ var ix *modindex.Index
+ if src, ok := source.(*ProcessEnvSource); ok {
+ var err error
+ if ix, err = modindex.Read(src.env.Env["GOMODCACHE"]); err != nil {
+ ix = nil // don't use it if there was an error
+ }
+ }
+
if ix != nil {
src, ok := p.source.(*ProcessEnvSource)
if ok {
Change information
- M internal/imports/fix.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Message from Gopher Robot![]( "Open in Gerrit")
Congratulations on opening your first change. Thank you for your contribution!
Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded the change for review![]( "Open in Gerrit")
Commit message
internal/imports: read the module index only where it is used
getFixesWithSource read the GOMODCACHE index on entry, but ix is not used until the third pass, after three early returns. A file whose imports are already complete returns from the first pass, having read and parsed the entire index for nothing.
The cost is per file, not per run: the imports.Process wrapper builds a fresh ProcessEnv for each call and modindex.Read is not memoized, so a tool that formats a tree re-reads the index once per file. With a 60MB index, goimports -l over 500 already-formatted files in x/tools took 41.5s and 233MB peak RSS; reading the index at its point of first use instead brings that to 0.47s and 20MB.
Files that do need an import resolved still read the index and are unaffected. Verified by comparing output before and after over 8220 files, and over a mutation corpus of 250 files in a non-vendored module with one third-party import deleted from each, which exercises addExternalCandidates; output is identical in every case.
This applies a change requested during review of CL 767881 (patch set 23) that was acknowledged but did not reach the submitted revision.
Fixes golang/go#80774
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Gopher Robot added 1 comment![]( "Open in Gerrit")
I spotted some possible problems with your PR:
1. You have a long 391 character line in the commit message body. Please add line breaks to long lines that should be wrapped. Lines in the commit message body should be wrapped at ~76 characters unless needed for things like URLs or tables. (Note: GitHub might render long lines as soft-wrapped, so double-check in the Gerrit commit message shown above.)
Please address any problems by updating the GitHub PR.
When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above. These findings are based on heuristics; if a finding does not apply, briefly reply here saying so.
To update the commit title or commit message body shown here in Gerrit, you must edit the GitHub PR title and PR description (the first comment) in the GitHub web interface using the 'Edit' button or 'Edit' menu entry there. Note: pushing a new commit to the PR will not automatically update the commit message used by Gerrit.
For more details, see:
- [how to update commit messages](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message) for PRs imported into Gerrit.
- the Go project's [conventions for commit messages](https://go.dev/doc/contribute#commit_messages) that you should follow.
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brad Moylan (Gerrit)
Brad Moylan abandoned this change.
Brad Moylan abandoned this change![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brad Moylan (Gerrit)
Brad Moylan added 1 comment![]( "Open in Gerrit")
I spotted some possible problems with your PR:
1. You have a long 391 character line in the commit message body. Please add line breaks to long lines that should be wrapped. Lines in the commit message body should be wrapped at ~76 characters unless needed for things like URLs or tables. (Note: GitHub might render long lines as soft-wrapped, so double-check in the Gerrit commit message shown above.)Please address any problems by updating the GitHub PR.
When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above. These findings are based on heuristics; if a finding does not apply, briefly reply here saying so.
To update the commit title or commit message body shown here in Gerrit, you must edit the GitHub PR title and PR description (the first comment) in the GitHub web interface using the 'Edit' button or 'Edit' menu entry there. Note: pushing a new commit to the PR will not automatically update the commit message used by Gerrit.
For more details, see:
- [how to update commit messages](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message) for PRs imported into Gerrit.
- the Go project's [conventions for commit messages](https://go.dev/doc/contribute#commit_messages) that you should follow.
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Done
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Hongxiang Jiang
- Peter Weinberger
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Hongxiang Jiang
- Peter Weinberger
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |