fix: speed up gopls lint with install optimization and optional parallelism #123
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
The gopls check was slow (~3.5 minutes) because it processed all ~2800 Go source files sequentially. This change: 1. Installs gopls once instead of using 'go run' each time 2. Uses xargs with parallel execution (-P nproc) to process files in batches of 100 across all available CPU cores This reduces the lint time from ~3.5 minutes to ~1 minute (about 68% faster). Closes #121 Co-Authored-By: Greg Slepak <contact@taoeffect.com>
devin-ai-integration
bot
force-pushed
the
devin/1769030545-fix-gopls-lint-performance
branch
from
622d3fc to
9a94079
Compare
devin-ai-integration
bot
changed the title
Add lint-frontend and lint-backend steps to run before their respective test steps, ensuring linting passes before tests run. Co-Authored-By: Greg Slepak <contact@taoeffect.com>
Linting is already handled by pull-compliance.yml which runs on all PRs. No need to duplicate lint steps in pull-tests.yml. Co-Authored-By: Greg Slepak <contact@taoeffect.com>
- Add cross-platform CPU detection (nproc for Linux, sysctl for macOS) - Add verification that gopls was installed successfully - Use null-delimited format (-0) with xargs to handle filenames with spaces - Remove stderr suppression from go install to surface installation errors Co-Authored-By: Greg Slepak <contact@taoeffect.com>
pedrogaudencio
left a comment
pedrogaudencio
left a comment
There was a problem hiding this comment.
Thank you for looking into this - switching from go run to installing gopls is a nice win and makes it cleaner.
There is one concern tho: the xargs -P "$NPROC" -n 100 approach ends up launching a bunch of gopls check processes at the same time. On my laptop that caused heavy memory/swap pressure (and lag) and actually made make lint slower overall, even tho it’s “more parallel”.
I’d suggest we keep the “install + reuse gopls” part, but run run gopls check in a single invocation (or sequential batches). If we still want parallelism, maybe make it a configurable and default to something conservative (e.g. GOPLS_PARALLEL=1 locally, and higher in CI).
I'm happy to help adjust the script to “install once + sequential check” vs “configurable parallelism”.
Address feedback from pedrogaudencio: parallel gopls processes can cause memory/swap pressure on local machines. Changed to: - Default GOPLS_PARALLEL=1 (sequential) for local development - CI can set GOPLS_PARALLEL to a higher value if desired - Removed automatic CPU detection since parallelism is now opt-in The 'install gopls once' optimization is kept, which still provides a speedup over the original 'go run' approach. Co-Authored-By: Greg Slepak <contact@taoeffect.com>
devin-ai-integration
bot
changed the title
|
Thanks for the feedback @pedrogaudencio! I've updated the script to address your concern:
The "install gopls once" optimization is kept, which still provides a speedup over the original |
|
Closing due to inactivity for more than 7 days. Configure here. |
|
@pedrogaudencio Should we just give it a try, parallelism of 4 in CI via that env var? |
.github/workflows/pull-compliance.yml, in env block around lines 14-27
|
- Use GOBIN if set, falling back to GOPATH/bin for gopls binary path - Add GOPLS_PARALLEL=4 to CI workflow for faster lint times Addresses feedback from @eighttrigrams Co-Authored-By: Greg Slepak <contact@taoeffect.com>
|
Done! I've implemented both suggestions:
GOBIN_DIR=$("$GO" env GOBIN)
if [[ -z "$GOBIN_DIR" ]]; then
GOBIN_DIR=$("$GO" env GOPATH)/bin
fi
GOPLS_BIN="$GOBIN_DIR/gopls"
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
|
||
| # Install gopls if not already installed, then use the installed binary for | ||
| # faster execution. Using 'go run' each time adds overhead. | ||
| "$GO" install "$GOPLS_PACKAGE" |
There was a problem hiding this comment.
🟡 go install failure is silently ignored when a stale gopls binary exists
The script uses set -uo pipefail but not set -e, so the go install command on line 12 can fail without causing the script to exit.
Click to expand
Problem
The subsequent check at line 22 ([[ ! -x "$GOPLS_BIN" ]]) only verifies that some gopls binary exists and is executable - it doesn't verify that the current go install command succeeded.
If:
- An older version of gopls was previously installed
- The current
go install "$GOPLS_PACKAGE"fails (e.g., network error, Go module error) - The script will silently continue using the stale/outdated gopls binary
Impact
This contradicts the PR's stated intent to "Verify gopls installation succeeded before proceeding". Users could unknowingly run linting with an outdated version of gopls that may have different behavior, miss certain issues, or report false positives.
Example scenario
# First run installs gopls v0.19.0
# Later, GOPLS_PACKAGE is updated to v0.20.0
# If go install fails, script silently uses v0.19.0Recommendation: Check the exit code of go install explicitly, e.g.:
if ! "$GO" install "$GOPLS_PACKAGE"; then
echo "Error: Failed to install gopls" >&2
exit 1
fiOr add set -e to the script options on line 2.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
claude says on a clean lint this would then somehow fail with "set -euo pipefail", some kind of pitfall.
i say we let our experience with it speak rather than go into another rabbithole here, which is the intricacies of pipefail
There was a problem hiding this comment.
Sounds good - I'll leave the current implementation as-is. The explicit check for the binary existence ([[ ! -x "$GOPLS_BIN" ]]) should catch the most common failure cases, and we can iterate if real-world issues come up.
3 participants
Closes #121
Summary
The
gopls checkstep inmake lintwas slow (~3.5 minutes) because it rango runeach time, recompiling gopls. This PR speeds it up by:go installinstead of usinggo runeach timeGOPLS_PARALLELenv var (defaults to 1 to avoid memory pressure)GOPLS_PARALLEL=4in CI for faster lint timesBefore: ~3.5 minutes
After: ~3 minutes with default settings (sequential), faster with
GOPLS_PARALLELset higherChanges
Modified
tools/lint-go-gopls.shto:go installbefore running checksGOBINenvironment variable when locating gopls binaryGOPLS_PARALLELenv var (default: 1)xargs -0 -P "$PARALLEL" -n 100with null-delimited input to handle filenames with spacesModified
.github/workflows/pull-compliance.ymlto:GOPLS_PARALLEL: 4for thelint-backendjobTesting
Verified that the linter still catches errors by intentionally adding a syntax error to
services/user/user.goand confirming it was detected.Updates since last revision
GOPLS_PARALLELenv var, defaulting to 1 (sequential).GOBINhandling so gopls binary is found correctly whenGOBINis set.GOPLS_PARALLEL=4to CI workflow as suggested by @eighttrigrams.Human Review Checklist
go installapproach works correctly in CI environmentsGOBINhandling works whenGOBINenvironment variable is setGOPLS_PARALLEL=4in CI doesn't cause memory issues on GitHub Actions runners2>/dev/null) - this is intentional to filter noise but could hide some edge case errorsLink to Devin run: https://app.devin.ai/sessions/c29881ff27ee429e9b2592f7f655c998
Requested by: Greg Slepak (@taoeffect)