From f8a7325d2f64b8b16a64158884f92bff384e7fb1 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Mon, 20 Mar 2023 19:35:18 -0400 Subject: [PATCH] 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 gopls-CI: kokoro Run-TryBot: Robert Findley Reviewed-by: Alan Donovan --- gopls/internal/lsp/cache/analysis.go | 16 +++++ gopls/internal/lsp/cache/errors.go | 8 ++- gopls/internal/lsp/cache/errors_test.go | 78 +++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 3 deletions(-) diff --git a/gopls/internal/lsp/cache/analysis.go b/gopls/internal/lsp/cache/analysis.go index 5d8616b5e7b..25e4a018a3d 100644 --- 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 index f14631b350a..fcb5c5cbaac 100644 --- 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 index 43cc03f78f7..933e9e87e5d 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 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) + } +}