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: golang/go#29766 (comment) 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 <[email protected]> gopls-CI: kokoro <[email protected]> Run-TryBot: Robert Findley <[email protected]> Reviewed-by: Alan Donovan <[email protected]>
golang · Mar 21, 2023 · f8a7325 · f8a7325
1 parent df92f17
commit f8a7325
Show file tree

Hide file tree

Showing 3 changed files with 99 additions and 3 deletions.
diff --git a/gopls/internal/lsp/cache/analysis.go b/gopls/internal/lsp/cache/analysis.go
@@ -1145,12 +1145,28 @@ func mustEncode(x interface{}) []byte {
 	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
@@ -249,13 +249,13 @@ func encodeDiagnostics(srcDiags []*source.Diagnostic) []byte {
 		}
 		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 @@ func decodeDiagnostics(data []byte) []*source.Diagnostic {
 				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 @@ func decodeDiagnostics(data []byte) []*source.Diagnostic {
 			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
@@ -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 TestParseErrorMessage(t *testing.T) {
 		})
 	}
 }
+
+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)
+	}
+}