[tools] gopls/internal/test: attempt to fix BenchmarkSemanticTokens
6 views
Skip to first unread message
Madeline Kalil (Gerrit)
Jun 25, 2026, 5:37:54 PM (2 days ago) Jun 25
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Madeline Kalil has uploaded the change for review![]( "Open in Gerrit")
Commit message
gopls/internal/test: attempt to fix BenchmarkSemanticTokens
The reason we haven't been generating new benchmarks
for x/tools is that they're all failing:
https://ci.chromium.org/ui/p/golang/builders/ci/x_tools-go1.26-linux-arm64_c4as16-perf_vs_gopls_0_11?status=FAILURE
Specifically, the BenchmarkSemanticTokens is failing with:
"panic: runtime error: index out of range [17] with length 0"
https://logs.chromium.org/logs/golang/buildbucket/cr-buildbucket/8677996571488419617/+/u/step/27/log/3
The panic is coming from interpretTokens when it tries to access an
index in legend.TokenTypes. legend.TokenTypes is only empty when
SemanticTokensProvider is nil, if the editor environment is
initialized to not provide semantic tokens. However, the BenchmarkSemanticTokens
does set semanticTokens = true when calling newEnv, so this is confusing.
The benchmarks don't fail locally, so maybe there is some concurrency
issue with the env that it doesn't initialize the semantic token
provider correctly. We should call env.Close() to cleanup the env
after the benchmark run finishes. Do this in BenchmarkSemanticTokens
and also in BenchmarkDiagnosePackageFiles.
We could also make interpretTokens safer by checking if legend.TokenTypes
is empty, but I'm not sure.
For golang/go#80113
Change-Id: Ib6f63aa423508473c1a3b9a918b1f77636ef663a
Change diff
diff --git a/gopls/internal/test/integration/bench/diagnostic_test.go b/gopls/internal/test/integration/bench/diagnostic_test.go
index 39b3a9c..3e4cee9 100644
--- a/gopls/internal/test/integration/bench/diagnostic_test.go
+++ b/gopls/internal/test/integration/bench/diagnostic_test.go
@@ -29,6 +29,7 @@
"pullDiagnostics": true, // currently required for pull diagnostic support
},
}, "diagnosePackageFiles", false)
+ defer env.Close()
// 10 arbitrary files in a single package.
files := []string{
diff --git a/gopls/internal/test/integration/bench/semtok_test.go b/gopls/internal/test/integration/bench/semtok_test.go
index 4f9d290..fc162d5 100644
--- a/gopls/internal/test/integration/bench/semtok_test.go
+++ b/gopls/internal/test/integration/bench/semtok_test.go
@@ -17,6 +17,7 @@
"semanticTokens": true,
},
}, "SemanticTokens", false)
+ defer env.Close()
env.Await(InitialWorkspaceLoad)
Change information
Files:
- M gopls/internal/test/integration/bench/diagnostic_test.go
- M gopls/internal/test/integration/bench/semtok_test.go
Change size: XS
Delta: 2 files changed, 2 insertions(+), 0 deletions(-)
Related details
Attention set is empty
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Jun 25, 2026, 5:38:22 PM (2 days ago) Jun 25
to goph...@pubsubhelper.golang.org, Alan Donovan, golang-co...@googlegroups.com
Attention needed from Alan Donovan
Madeline Kalil voted Commit-Queue+1![]( "Open in Gerrit")
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Jun 26, 2026, 1:19:00 PM (18 hours ago) Jun 26
to Madeline Kalil, goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, golang-co...@googlegroups.com
Attention needed from Madeline Kalil
Alan Donovan added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 1 (Latest):
Alan Donovan . resolved
Thanks for investigating!
Should every newEnv call be paired with a call to Close? I count six calls to Close and 7 to newEnv.
It might be less error-prone to change the type of newEnv to return a pair `(env *Env, close func())` so that the caller must think about the close function. Then no Close method is needed.
Related details
Attention is currently required from:
- Madeline Kalil
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Jun 26, 2026, 1:37:55 PM (17 hours ago) Jun 26
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Madeline Kalil
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Madeline Kalil uploaded patch set #2 to this change.
Following approvals got outdated and were removed:
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
Attention is currently required from:
- Madeline Kalil
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Jun 26, 2026, 1:39:21 PM (17 hours ago) Jun 26
to goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, Alan Donovan, golang-co...@googlegroups.com
Attention needed from Alan Donovan
Madeline Kalil voted and added 1 comment![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
Alan Donovan . resolved
Thanks for investigating!
Should every newEnv call be paired with a call to Close? I count six calls to Close and 7 to newEnv.
It might be less error-prone to change the type of newEnv to return a pair `(env *Env, close func())` so that the caller must think about the close function. Then no Close method is needed.
Madeline Kalil
Yep, each newEnv should have a corresponding Close, I thought I got them all.
That sounds good, but we can't delete Close because it's also used in cleanup() which runs on the shared repos after benchmarks have finished.
Related details
Attention is currently required from:
- Alan Donovan
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Jun 26, 2026, 1:59:51 PM (17 hours ago) Jun 26
to Madeline Kalil, goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, golang-co...@googlegroups.com
Attention needed from Madeline Kalil
Alan Donovan added 2 comments![]( "Open in Gerrit")
File gopls/internal/test/integration/bench/repo_test.go
Line 230, Patchset 2 (Latest):
// must be called on the Env when it is no longer needed.Alan Donovan . unresolved
called when the Env is
Line 253, Patchset 2 (Latest):
env.Close()Alan Donovan . unresolved
```
if err := env.Close(); err != nil {
tb.Log(err)
}
```
so we don't lose this information from the log.
Related details
Attention is currently required from:
- Madeline Kalil
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Jun 26, 2026, 2:13:17 PM (17 hours ago) Jun 26
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Madeline Kalil
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Madeline Kalil uploaded patch set #3 to this change.
Following approvals got outdated and were removed:
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
Attention is currently required from:
- Madeline Kalil
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Jun 26, 2026, 2:13:35 PM (17 hours ago) Jun 26
to goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, Alan Donovan, golang-co...@googlegroups.com
Attention needed from Alan Donovan
Madeline Kalil added 2 comments![]( "Open in Gerrit")
File gopls/internal/test/integration/bench/repo_test.go
Line 230, Patchset 2:
// must be called on the Env when it is no longer needed.Alan Donovan . resolved
called when the Env is
Madeline Kalil
Done
Line 253, Patchset 2:
env.Close()Alan Donovan . unresolved
```
if err := env.Close(); err != nil {
tb.Log(err)
}
```
so we don't lose this information from the log.
Madeline Kalil
*Env.Close doesn't return anything and already does TB.Errorf on error (*repo.Close returns error). Should I change that behavior?
Related details
Attention is currently required from:
- Alan Donovan
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Jun 26, 2026, 2:15:41 PM (17 hours ago) Jun 26
to Madeline Kalil, goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, golang-co...@googlegroups.com
Attention needed from Madeline Kalil
Alan Donovan added 1 comment![]( "Open in Gerrit")
File gopls/internal/test/integration/bench/repo_test.go
Line 253, Patchset 2:
env.Close()Alan Donovan . unresolved
Madeline Kalil```
if err := env.Close(); err != nil {
tb.Log(err)
}
```
so we don't lose this information from the log.
*Env.Close doesn't return anything and already does TB.Errorf on error (*repo.Close returns error). Should I change that behavior?
Alan Donovan
No; sorry, I got confused by seeing repo.Close below, which is only for use after sharedEnv, I think. So you can change the code to simply `return env, env.Close`.
Related details
Attention is currently required from:
- Madeline Kalil
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Jun 26, 2026, 2:17:03 PM (17 hours ago) Jun 26
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention needed from Madeline Kalil
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Madeline Kalil uploaded patch set #4 to this change.
Related details
Attention is currently required from:
- Madeline Kalil
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Jun 26, 2026, 2:17:24 PM (17 hours ago) Jun 26
to goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, Alan Donovan, golang-co...@googlegroups.com
Attention needed from Alan Donovan
Madeline Kalil voted and added 1 comment![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
1 comment
File gopls/internal/test/integration/bench/repo_test.go
Line 253, Patchset 2:
env.Close()Alan Donovan . resolved
Madeline Kalil```
if err := env.Close(); err != nil {
tb.Log(err)
}
```
so we don't lose this information from the log.
Alan Donovan*Env.Close doesn't return anything and already does TB.Errorf on error (*repo.Close returns error). Should I change that behavior?
No; sorry, I got confused by seeing repo.Close below, which is only for use after sharedEnv, I think. So you can change the code to simply `return env, env.Close`.
Attention is currently required from:
- Alan Donovan
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Jun 26, 2026, 2:17:41 PM (17 hours ago) Jun 26
to goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, Alan Donovan, golang-co...@googlegroups.com
Attention needed from Alan Donovan
Madeline Kalil voted Auto-Submit+1![]( "Open in Gerrit")
| Auto-Submit | +1 |
Related details
Attention is currently required from:
- Alan Donovan
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Jun 26, 2026, 2:23:07 PM (17 hours ago) Jun 26
to Madeline Kalil, goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, golang-co...@googlegroups.com
Attention needed from Madeline Kalil
Alan Donovan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alan Donovan
| Code-Review | +2 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Madeline Kalil
Submit Requirements:
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)
Jun 26, 2026, 2:40:53 PM (16 hours ago) Jun 26
to Madeline Kalil, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, golang...@luci-project-accounts.iam.gserviceaccount.com, Alan Donovan, golang-co...@googlegroups.com
Gopher Robot submitted the change![]( "Open in Gerrit")
Change information
Commit message:
gopls/internal/test: attempt to fix BenchmarkSemanticTokens
The reason we haven't been generating new benchmarks
for x/tools is that they're all failing:
https://ci.chromium.org/ui/p/golang/builders/ci/x_tools-go1.26-linux-arm64_c4as16-perf_vs_gopls_0_11?status=FAILURE
Specifically, the BenchmarkSemanticTokens is failing with:
"panic: runtime error: index out of range [17] with length 0"
https://logs.chromium.org/logs/golang/buildbucket/cr-buildbucket/8677996571488419617/+/u/step/27/log/3
The panic is coming from interpretTokens when it tries to access an
index in legend.TokenTypes. legend.TokenTypes is only empty when
SemanticTokensProvider is nil, if the editor environment is
initialized to not provide semantic tokens. However, the BenchmarkSemanticTokens
does set semanticTokens = true when calling newEnv, so this is confusing.
The benchmarks don't fail locally, so maybe there is some concurrency
issue with the env that it doesn't initialize the semantic token
provider correctly. We should call env.Close() to cleanup the env
after the benchmark run finishes. Do this in BenchmarkSemanticTokens
and also in BenchmarkDiagnosePackageFiles.
We could also make interpretTokens safer by checking if legend.TokenTypes
is empty, but I'm not sure.
For golang/go#80113
Change-Id: Ib6f63aa423508473c1a3b9a918b1f77636ef663a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/794620
Auto-Submit: Madeline Kalil <mka...@google.com>
Reviewed-by: Alan Donovan <adon...@google.com>
LUCI-TryBot-Result: golang...@luci-project-accounts.iam.gserviceaccount.com <golang...@luci-project-accounts.iam.gserviceaccount.com>
Files:
- M gopls/internal/test/integration/bench/completion_test.go
- M gopls/internal/test/integration/bench/diagnostic_test.go
- M gopls/internal/test/integration/bench/didchange_test.go
- M gopls/internal/test/integration/bench/imports_test.go
- M gopls/internal/test/integration/bench/iwl_test.go
- M gopls/internal/test/integration/bench/repo_test.go
- M gopls/internal/test/integration/bench/semtok_test.go
Change size: S
Delta: 7 files changed, 20 insertions(+), 18 deletions(-)
Branch: refs/heads/master
Submit Requirements:
Code-Review: +2 by Alan Donovan
TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages