gopls/internal/lsp/cache: use json encoding for diagnostics
Benchmarking demonstrated that encoding and decoding diagnostics when
diagnosting the workspace consumes a disproportionate amount of time,
especially when the diagnostic sets are almost empty.
The encoding/gob package is not intended for thousands of small encoding
and decoding operations at low latency, as described here:
https://github.com/golang/go/issues/29766#issuecomment-454926474
Using JSON improved observed encoding performance significantly.
Also, add a test... and fix the bugs uncovered by the test.
For golang/go#57987
Change-Id: I52af8a680ed20cf07e6c61ea496bcf9fd761e6da
Reviewed-on: https://go-review.googlesource.com/c/tools/+/477977
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
diff --git a/gopls/internal/lsp/cache/analysis.go b/gopls/internal/lsp/cache/analysis.go
index 5d8616b..25e4a01 100644
--- a/gopls/internal/lsp/cache/analysis.go
+++ b/gopls/internal/lsp/cache/analysis.go
@@ -1145,12 +1145,28 @@
return buf.Bytes()
}
+// TODO(rfindley): based on profiling, we should consider using JSON encoding
+// throughout, rather than gob encoding.
+func mustJSONEncode(x interface{}) []byte {
+ data, err := json.Marshal(x)
+ if err != nil {
+ log.Fatalf("internal error marshalling %T: %v", data, err)
+ }
+ return data
+}
+
func mustDecode(data []byte, ptr interface{}) {
if err := gob.NewDecoder(bytes.NewReader(data)).Decode(ptr); err != nil {
log.Fatalf("internal error decoding %T: %v", ptr, err)
}
}
+func mustJSONDecode(data []byte, ptr interface{}) {
+ if err := json.Unmarshal(data, ptr); err != nil {
+ log.Fatalf("internal error unmarshalling %T: %v", ptr, err)
+ }
+}
+
// -- data types for serialization of analysis.Diagnostic and source.Diagnostic --
type gobDiagnostic struct {
diff --git a/gopls/internal/lsp/cache/errors.go b/gopls/internal/lsp/cache/errors.go
index f14631b..fcb5c5c 100644
--- a/gopls/internal/lsp/cache/errors.go
+++ b/gopls/internal/lsp/cache/errors.go
@@ -249,13 +249,13 @@
}
gobDiags = append(gobDiags, gobDiag)
}
- return mustEncode(gobDiags)
+ return mustJSONEncode(gobDiags)
}
// decodeDiagnostics decodes the given gob-encoded diagnostics.
func decodeDiagnostics(data []byte) []*source.Diagnostic {
var gobDiags []gobDiagnostic
- mustDecode(data, &gobDiags)
+ mustJSONDecode(data, &gobDiags)
var srcDiags []*source.Diagnostic
for _, gobDiag := range gobDiags {
var srcFixes []source.SuggestedFix
@@ -276,7 +276,7 @@
srcFix.Edits[uri] = append(srcFix.Edits[uri], srcEdit)
}
if gobCmd := gobFix.Command; gobCmd != nil {
- gobFix.Command = &gobCommand{
+ srcFix.Command = &protocol.Command{
Title: gobCmd.Title,
Command: gobCmd.Command,
Arguments: gobCmd.Arguments,
@@ -293,6 +293,8 @@
URI: gobDiag.Location.URI.SpanURI(),
Range: gobDiag.Location.Range,
Severity: gobDiag.Severity,
+ Code: gobDiag.Code,
+ CodeHref: gobDiag.CodeHref,
Source: source.AnalyzerErrorKind(gobDiag.Source),
Message: gobDiag.Message,
Tags: gobDiag.Tags,
diff --git a/gopls/internal/lsp/cache/errors_test.go b/gopls/internal/lsp/cache/errors_test.go
index 43cc03f..933e9e8 100644
--- a/gopls/internal/lsp/cache/errors_test.go
+++ b/gopls/internal/lsp/cache/errors_test.go
@@ -5,8 +5,14 @@
package cache
import (
+ "encoding/json"
"strings"
"testing"
+
+ "github.com/google/go-cmp/cmp"
+ "golang.org/x/tools/gopls/internal/lsp/protocol"
+ "golang.org/x/tools/gopls/internal/lsp/source"
+ "golang.org/x/tools/gopls/internal/span"
)
func TestParseErrorMessage(t *testing.T) {
@@ -50,3 +56,75 @@
})
}
}
+
+func TestDiagnosticEncoding(t *testing.T) {
+ diags := []*source.Diagnostic{
+ {}, // empty
+ {
+ URI: "file///foo",
+ Range: protocol.Range{
+ Start: protocol.Position{Line: 4, Character: 2},
+ End: protocol.Position{Line: 6, Character: 7},
+ },
+ Severity: protocol.SeverityWarning,
+ Code: "red",
+ CodeHref: "https://go.dev",
+ Source: "test",
+ Message: "something bad happened",
+ Tags: []protocol.DiagnosticTag{81},
+ Related: []protocol.DiagnosticRelatedInformation{
+ {
+ Location: protocol.Location{
+ URI: "file:///other",
+ Range: protocol.Range{
+ Start: protocol.Position{Line: 3, Character: 6},
+ End: protocol.Position{Line: 4, Character: 9},
+ },
+ },
+ Message: "psst, over here",
+ },
+ },
+
+ // Fields below are used internally to generate quick fixes. They aren't
+ // part of the LSP spec and don't leave the server.
+ SuggestedFixes: []source.SuggestedFix{
+ {
+ Title: "fix it!",
+ Edits: map[span.URI][]protocol.TextEdit{
+ "file:///foo": {{
+ Range: protocol.Range{
+ Start: protocol.Position{Line: 4, Character: 2},
+ End: protocol.Position{Line: 6, Character: 7},
+ },
+ NewText: "abc",
+ }},
+ "file:///other": {{
+ Range: protocol.Range{
+ Start: protocol.Position{Line: 4, Character: 2},
+ End: protocol.Position{Line: 6, Character: 7},
+ },
+ NewText: "!@#!",
+ }},
+ },
+ Command: &protocol.Command{
+ Title: "run a command",
+ Command: "gopls.fix",
+ Arguments: []json.RawMessage{json.RawMessage(`{"a":1}`)},
+ },
+ ActionKind: protocol.QuickFix,
+ },
+ },
+ },
+ {
+ URI: "file//bar",
+ // other fields tested above
+ },
+ }
+
+ data := encodeDiagnostics(diags)
+ diags2 := decodeDiagnostics(data)
+
+ if diff := cmp.Diff(diags, diags2); diff != "" {
+ t.Errorf("decoded diagnostics do not match (-original +decoded):\n%s", diff)
+ }
+}
