[tools] gopls/internal/settings: expose file cache budget as setting
Pontus Leitzler (Gerrit)
Pontus Leitzler has uploaded the change for review![]( "Open in Gerrit")
Commit message
gopls/internal/settings: expose file cache budget as setting
The default file cache buget of 1GB can be exhaused in rare cases,
such as when opening a single-module monorepo with many packages.
This leads to the premature eviction of cache data, indirectly
causing new typecheck runs of arbitrary packages (for example).
Large packages, like common cloud vendor SDKs, can take several
seconds to re-analyze, consuming resources unnecessarily.
We now expose the budget as an experimental setting to allow
users to increase the budget.
Change diff
diff --git a/gopls/doc/settings.md b/gopls/doc/settings.md
index dd8a5d5..be9bc9f 100644
--- a/gopls/doc/settings.md
+++ b/gopls/doc/settings.md
@@ -624,6 +624,16 @@
Default: `"all"`.
+<a id='fileCacheBudget'></a>
+### `fileCacheBudget int`
+
+**This setting is experimental and may be deleted.**
+
+fileCacheBudget sets a soft limit on the file cache size in bytes.
+If zero, the default budget is used.
+
+Default: `0`.
+
<a id='verboseOutput'></a>
### `verboseOutput bool`
diff --git a/gopls/internal/doc/api.json b/gopls/internal/doc/api.json
index 108ecdb..1c9141d 100644
--- a/gopls/internal/doc/api.json
+++ b/gopls/internal/doc/api.json
@@ -2237,6 +2237,20 @@
"DeprecationMessage": ""
},
{
+ "Name": "fileCacheBudget",
+ "Type": "int",
+ "Doc": "fileCacheBudget sets a soft limit on the file cache size in bytes.\nIf zero, the default budget is used.\n",
+ "EnumKeys": {
+ "ValueType": "",
+ "Keys": null
+ },
+ "EnumValues": null,
+ "Default": "0",
+ "Status": "experimental",
+ "Hierarchy": "",
+ "DeprecationMessage": ""
+ },
+ {
"Name": "verboseOutput",
"Type": "bool",
"Doc": "verboseOutput enables additional debug logging.\n",
diff --git a/gopls/internal/server/general.go b/gopls/internal/server/general.go
index d511de3..7bd684c 100644
--- a/gopls/internal/server/general.go
+++ b/gopls/internal/server/general.go
@@ -24,6 +24,7 @@
"golang.org/x/tools/gopls/internal/cache"
"golang.org/x/tools/gopls/internal/debug"
debuglog "golang.org/x/tools/gopls/internal/debug/log"
+ "golang.org/x/tools/gopls/internal/filecache"
"golang.org/x/tools/gopls/internal/protocol"
"golang.org/x/tools/gopls/internal/protocol/semtok"
"golang.org/x/tools/gopls/internal/settings"
@@ -67,6 +68,10 @@
}
options.ForClientCapabilities(params.ClientInfo, params.Capabilities)
+ if options.FileCacheBudget > 0 {
+ filecache.SetBudget(int64(options.FileCacheBudget))
+ }
+
if options.ShowBugReports {
// Report the next bug that occurs on the server.
bug.Handle(func(b bug.Bug) {
diff --git a/gopls/internal/server/workspace.go b/gopls/internal/server/workspace.go
index 7f5dc50..fac5ba5 100644
--- a/gopls/internal/server/workspace.go
+++ b/gopls/internal/server/workspace.go
@@ -13,6 +13,7 @@
"golang.org/x/tools/gopls/internal/cache"
"golang.org/x/tools/gopls/internal/file"
+ "golang.org/x/tools/gopls/internal/filecache"
"golang.org/x/tools/gopls/internal/golang/completion"
"golang.org/x/tools/gopls/internal/protocol"
"golang.org/x/tools/gopls/internal/settings"
@@ -137,6 +138,10 @@
// An options change may have affected the detected Go version.
s.checkViewGoVersions()
+ if options.FileCacheBudget > 0 {
+ filecache.SetBudget(int64(options.FileCacheBudget))
+ }
+
return nil
}
diff --git a/gopls/internal/settings/settings.go b/gopls/internal/settings/settings.go
index 17a3e5a..6bf4806 100644
--- a/gopls/internal/settings/settings.go
+++ b/gopls/internal/settings/settings.go
@@ -658,6 +658,10 @@
UIOptions
FormattingOptions
+ // FileCacheBudget sets a soft limit on the file cache size in bytes.
+ // If zero, the default budget is used.
+ FileCacheBudget int `status:"experimental"`
+
// VerboseOutput enables additional debug logging.
VerboseOutput bool `status:"debug"`
}
@@ -1265,6 +1269,9 @@
case "local":
return nil, setString(&o.Local, value)
+ case "fileCacheBudget":
+ return setInt(&o.FileCacheBudget, value)
+
case "verboseOutput":
return setBool(&o.VerboseOutput, value)
@@ -1673,3 +1680,28 @@
}
return "", fmt.Errorf("invalid option %q for enum", str)
}
+
+func setInt(dest *int, value any) ([]CounterPath, error) {
+ i, err := asInt(value)
+ if err != nil {
+ return nil, err
+ }
+ *dest = i
+ return []CounterPath{{fmt.Sprint(i)}}, nil
+}
+
+func asInt(value any) (int, error) {
+ switch v := value.(type) {
+
+ case float64:
+ // Note that this is what we expect since JSON unmarshalling
+ // into any produces float64 for numbers.
+ return int(v), nil
+ case int64:
+ return int(v), nil
+ case int:
+ return v, nil
+ default:
+ return 0, fmt.Errorf("invalid type %T (want number)", value)
+ }
+}
diff --git a/gopls/internal/test/integration/misc/filecache_test.go b/gopls/internal/test/integration/misc/filecache_test.go
new file mode 100644
index 0000000..0622ff3
--- /dev/null
+++ b/gopls/internal/test/integration/misc/filecache_test.go
@@ -0,0 +1,33 @@
+package misc
+
+import (
+ "testing"
+
+ "golang.org/x/tools/gopls/internal/filecache"
+ "golang.org/x/tools/gopls/internal/test/integration"
+)
+
+func TestFileCacheBudget(t *testing.T) {
+ const budget int64 = 2e9 // 2GB
+ integration.WithOptions(
+ integration.Settings(map[string]any{
+ "fileCacheBudget": budget,
+ }),
+ ).Run(t, "", func(t *testing.T, env *integration.Env) {
+ // Verify that budget is set.
+ if got := filecache.SetBudget(-1); got != int64(budget) {
+ t.Errorf("initial budget: got %d, want %d", got, budget)
+ }
+
+ const newBudget int64 = 1.5e9 // 1.5GB
+ cfg := env.Editor.Config()
+ cfg.Settings["fileCacheBudget"] = newBudget
+ env.ChangeConfiguration(cfg)
+ env.AfterChange()
+
+ // Verify that the budget was updated.
+ if got := filecache.SetBudget(-1); got != int64(newBudget) {
+ t.Errorf("updated budget: got %d, want %d", got, newBudget)
+ }
+ })
+}
Change information
- M gopls/doc/settings.md
- M gopls/internal/doc/api.json
- M gopls/internal/server/general.go
- M gopls/internal/server/workspace.go
- M gopls/internal/settings/settings.go
- A gopls/internal/test/integration/misc/filecache_test.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. |
Pontus Leitzler (Gerrit)
Pontus Leitzler voted Commit-Queue+1![]( "Open in Gerrit")
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)
Alan Donovan added 9 comments![]( "Open in Gerrit")
Thanks, this change looks reasonable, though I do wonder whether simply doubling (say) the default value might be good enough in practice.
(Is the monorepo that triggered the problem open source, BTW?)
// If zero, the default budget is used.// The cache may temporarily use more than this amount.
// Also, this parameter limits file contents; disk block usage
// as measured by du(1) may be significantly higher.
FileCacheBudget int `status:"experimental"`int64
case "fileCacheBudget":maxFileCacheBytes, perhaps (throughout)?
func setInt(dest *int, value any) ([]CounterPath, error) {setInt64 etc
// Note that this is what we expect since JSON unmarshalling
// into any produces float64 for numbers.Let's report an error if the value is not representable.
```
if !isFinite(v) { not finite }
if v != math.Trunc(v) { not an integer }
if v != float64(int64(v)) { not representable as int64 }
...
// isFinite reports whether f represents a finite rational value.
// It is equivalent to !math.IsNan(f) && !math.IsInf(f, 0).
func isFinite(f float64) bool {
return math.Abs(f) <= math.MaxFloat64
}
```
return 0, fmt.Errorf("invalid type %T (want number)", value)int64
const budget int64 = 2e9 // 2GBI'm a little afraid of this test because between L26 and L29 the filecache GC may evict the cache of the user (or CI service) running it. But I suppose it's pretty safe since it only raises the limit above the default value.
The test should restore the original value when it is finished though, as it will finish quickly but the other misc/ tests may run long enough that GC occurs.
Related details
- Pontus Leitzler
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Pontus Leitzler (Gerrit)
Pontus Leitzler uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Pontus Leitzler
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Pontus Leitzler (Gerrit)
Pontus Leitzler voted and added 9 comments![]( "Open in Gerrit")
Votes added by Pontus Leitzler
| Commit-Queue | +1 |
Thanks, this change looks reasonable, though I do wonder whether simply doubling (say) the default value might be good enough in practice.
(Is the monorepo that triggered the problem open source, BTW?)
Thanks for the quick review! The repo isn't open source unfortunately :(. I went for configurable as I'm still experimenting a bit with how much I'll need in practice, and didn't want to increase it for everyone. A gut feeling is that 1G should cover most use cases, might be something to add as telemetry in the future?
// The cache may temporarily use more than this amount.
// Also, this parameter limits file contents; disk block usage
// as measured by du(1) may be significantly higher.
Done
FileCacheBudget int `status:"experimental"`Pontus Leitzlerint64
Done
case "fileCacheBudget":maxFileCacheBytes, perhaps (throughout)?
Hm, would "max...." be good even if it is a soft limit? Maybe the docs covers it good enough. Just curious, I don't have any strong opinions either way.
func setInt(dest *int, value any) ([]CounterPath, error) {Pontus LeitzlersetInt64 etc
Done
// Note that this is what we expect since JSON unmarshalling
// into any produces float64 for numbers.Let's report an error if the value is not representable.
```
if !isFinite(v) { not finite }
if v != math.Trunc(v) { not an integer }
if v != float64(int64(v)) { not representable as int64 }
...// isFinite reports whether f represents a finite rational value.
// It is equivalent to !math.IsNan(f) && !math.IsInf(f, 0).
func isFinite(f float64) bool {
return math.Abs(f) <= math.MaxFloat64
}
```
Done
return 0, fmt.Errorf("invalid type %T (want number)", value)Pontus Leitzlerint64
Done
const budget int64 = 2e9 // 2GBI'm a little afraid of this test because between L26 and L29 the filecache GC may evict the cache of the user (or CI service) running it. But I suppose it's pretty safe since it only raises the limit above the default value.
The test should restore the original value when it is finished though, as it will finish quickly but the other misc/ tests may run long enough that GC occurs.
Good point. The intent was to have something larger than default so that it shouldn't affect anything. I could add comments to clarify that.
OTOH a user might have configured a larger limit than default causing 1.5G to evict too. I could change the values to something abnormal like 100 & 120 GB too?
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Pontus Leitzler (Gerrit)
Pontus Leitzler uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Pontus Leitzler (Gerrit)
Pontus Leitzler voted and added 2 comments![]( "Open in Gerrit")
Votes added by Pontus Leitzler
| Commit-Queue | +1 |
2 comments
case "fileCacheBudget":Pontus LeitzlermaxFileCacheBytes, perhaps (throughout)?
Hm, would "max...." be good even if it is a soft limit? Maybe the docs covers it good enough. Just curious, I don't have any strong opinions either way.
Updated
const budget int64 = 2e9 // 2GBPontus LeitzlerI'm a little afraid of this test because between L26 and L29 the filecache GC may evict the cache of the user (or CI service) running it. But I suppose it's pretty safe since it only raises the limit above the default value.
The test should restore the original value when it is finished though, as it will finish quickly but the other misc/ tests may run long enough that GC occurs.
Good point. The intent was to have something larger than default so that it shouldn't affect anything. I could add comments to clarify that.
OTOH a user might have configured a larger limit than default causing 1.5G to evict too. I could change the values to something abnormal like 100 & 120 GB too?
Added comment & increased to 50/55.
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |