Total CPU used by gopls is a critical metric for our users, yet was not
previously captured in any benchmark. This change uses the new pprof
parsing utilities added in CL 507885 to instrument a cpu_seconds
benchmark metric, for now just associated with the DidChange benchmark.
This is achieved via new LSP commands that start and stop a profile. The
benchmark runner uses these commands to bracket the critical section of
the benchmark, then parses the resulting profile for its total sampled
CPU.
Additionally, the benchmark runner is updated to actually check for the
existence of the custom command before instrumenting the new metric.
This allows it to be compatible with testing older versions of gopls.
The same technique is adopted for memstats metrics.
I only instrumented BenchmarkDidChange, because the profile file schema
is getting truly out of hand. I'll try to simplify it before
instrumenting all the other benchmarks, but want to do that in a
separate CL.
For golang/go#60926
Change-Id: Ia082bad49e8d30c567a7c07e050511d49b93738b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/508449
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This CL adds a command to report functions that are unreachable
from the main functions of applications and tests.
It uses the Rapid Type Analysis (RTA) algorithm to
compute reachability, and reports all functions referenced
by the SSA representation that were not found to be
reachable, grouped by package and sorted by position.
Also, a basic integration test.
Change-Id: Ide78b4e22d4f4066bf901e2d676e5058ca132827
Reviewed-on: https://go-review.googlesource.com/c/tools/+/507738
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
This CL is a substantial reorganization of the analysis driver to
ensure that export data is imported at most once per batch of packages
that are analyzed, instead of once per import edge. This greatly
reduces the amount of allocation and computation done during analysis.
In cache/analysis.go, Snapshot.Analyze (which now takes a set of
PackageIDs, instead of being called singly in a loop) constructs an
ephemeral DAG that mirrors the package graph, and then works in
parallel postorder over this graph doing analysis. It uses a single
FileSet for the whole batch of packages it creates. The subgraph
rooted at each node is effectively a types.Importer for that node,
as it represents the mapping from PackagePath to *types.Package.
We no longer bother with promises or invalidation. We rely on the fact
that the graph is relatively cheap to construct, cache hits are cheap
to process, and the whole process only occurs after an idle delay of
about a second.
Also:
- In internal/facts, optimize the fact decoder by using a callback.
Previously, it was spending a lot of time traversing the API of all
imports of a package to build a PackagePath-to-types.Package
mapping. For many packages in terraform-provider-aws this visits
over 1M objects (!!). But of course this is trivially computed from
the new representation.
- In internal/gcimporter, IImportShallow now uses a single callback to
get all the types.Package symbols from the client, potentially in
parallel (and that's what gopls does). The previous separation of
"create" and "populate" has gone away.
The analysis driver additionally exploits the getPackages callback to
efficiently read the package manifest of an export data file,
then abort with an error before proceeding to actually decode
the rest of the file.
With this change, we can process the internal/provider package of the
terraform-provider-aws repo in 20s cold, 4s hot. (Before, it would run
out of memory.)
$ go test -bench=InitialWorkspaceLoad/hashiform ./gopls/internal/regtest/bench
BenchmarkInitialWorkspaceLoad/hashiform-8 1 4014521793 ns/op 349570384 alloc_bytes 439230464 in_use_bytes 668992216 total_alloc_bytes
PASS
Fixesgolang/go#60621
Change-Id: Iadeb02f57eb19dcccb639857053b897a60e0a90e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/503195
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
This is an arbitrary fix to avoid a crash while exporting an empty
struct with no ambient package. Unfortunately, the struct encoding
assumes that there is an implied package for structs, which at least in
invalid code may not be the case.
Fixesgolang/go#60891
Change-Id: I8acd591ac15cdc1770d615fdca57dcb7bb9b7651
Reviewed-on: https://go-review.googlesource.com/c/tools/+/504556
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Add a test for goimports when using mod vendoring on windows, along with
a very subtle one-line fix.
Fixesgolang/go#56291
Change-Id: I2e45f70fc6dfa32164d4664acad886ec811474b8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/498695
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Fixes a crash resulting from trying to convert an unknown constant
value.
Fixesgolang/go#60605
Change-Id: If6b831b8fe2f9690b9f89e191b329eb7660f5e14
Reviewed-on: https://go-review.googlesource.com/c/tools/+/501209
TryBot-Bypass: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
This change adds to x/tools a workaround for a bug in go/types
that causes LookupFieldOrMethod and NewTypeSet to be inconsistent
wrt an ill-typed method (*T).f where T itself is a pointer.
The workaround is that, if Lookup fails, we walk the MethodSet.
Updates golang/go#60634Fixesgolang/go#60628
Change-Id: I87caa2ae077e5cdfa40b65a2f52e261384c91167
Reviewed-on: https://go-review.googlesource.com/c/tools/+/501197
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Previously, the expandEdit operation would expand to
the end of the line unconditionally, but this caused it
to gulp an extra line if it was already line-aligned.
This change causes it to do the expansion only if
the end is not line-aligned, or the replacement text
doesn't end with a newline.
Now, removing the fast path no longer causes tests to fail.
This also allows us to remove the logic added in CL 489695
to work around issue golang/go#59232.
Fixesgolang/go#60379Fixesgolang/go#59232
Change-Id: Ia40e4e3bb714d75acb95103a38e8c49a8ef456de
Reviewed-on: https://go-review.googlesource.com/c/tools/+/499377
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Peter Weinberger <pjw@google.com>
The fast-path "optimization" that skips the main
algorithm when the input is already line-aligned
failed to check that the replacement text consisted
of complete lines.
(Scare quotes because removing the "optimization" causes
tests to fail. See CL 499377 next in stack for why.)
Thanks to pjw for diagnosing the root cause and
providing the test case in CL 498975.
Fixesgolang/go#60379
Change-Id: I2ff92de4550754691442362b8a8932ee42971461
Reviewed-on: https://go-review.googlesource.com/c/tools/+/499376
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Peter Weinberger <pjw@google.com>
They were exported only because of unnecessary coupling
with another package, solved by copying.
Change-Id: I5f08ad9091b8fce10c2bac6383e020a3c45426f6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/498257
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This CL changes the implementation of the filecache to use
a scheme similar to that used by the go command's cache.
Instead of atomic rename(2), or flock(2), it instead relies
on the atomicity in practice of writes to small files (in our
case 32 bytes, the size of a SHA256 hash).
A cache entry now consists of two files, a kind="cas" file that
holds the cache value, keyed by its SHA256 hash, and an
index file, whose name is formed from the user-provided
kind and key, and whose content is the SHA256 hash that
is a key into the CAS.
Writes to the CAS may race, so we check the integrity of
everything we read back from it using SHA256.
Writes to the index files may also race, but we assume
that small writes are in practice atomic.
The memory-based LRU cache has beeen temporarily disabled
so that we can soak test the new implementation for a while.
We expect this to be significantly more reliable, and also faster.
Change-Id: I25cf341b90c985dcab015df770be579ea786bd06
Reviewed-on: https://go-review.googlesource.com/c/tools/+/495800
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
This change:
- updates the error message reported when the importer
recovers from a panic.
- updates the set of test input files to include examples
of the formats used in go1.16-go1.20.
- adds a recover handler to UImportData, for symmetry with
IImportData. This was exposed by the new test case.
- fixes an accidental shadowing bug that suppressed the
bundle format version check.
Fixesgolang/go#59179
Change-Id: Ib6c20fc15e2051481fccba593607a7df0e01bc74
Reviewed-on: https://go-review.googlesource.com/c/tools/+/494676
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Compilers and tools haven't produced it since go1.11,
several years ago now.
Change-Id: I5056c5bba81030a2eba5e3931190b8249524aed7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/494442
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
If the target is misbehaving and some changes unlock others,
then in general bisect is going to have a hard time and can fail
to identify the problem. It will usually say "target fails inconsistently".
One robustness improvement we can make is to use more bits than
necessary for exclusions, which reduces the chances of accidentally
excluding changes that simply didn't trigger this time around but
might still be part of a bug later. To do this, we calculate the minimum
number of bits needed to distinguish all the =y and =n changes
observed, round up to a number of hex digits, and then add another
digit for good measure.
Change-Id: I02354f281370806c3eb4d85911a6ca92fcfcae05
Reviewed-on: https://go-review.googlesource.com/c/tools/+/494276
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Add some visibility into goimports operations, by instrumenting spans in
top-level imports and gocommand operations.
This may be the first time we instrument non-gopls code in this way, but
it should be safe as other build targets (e.g. the goimports or
gopackages commands) do not set a global exporter, and therefore the
cost of event instrumentation should be minimal.
For golang/go#59216
Change-Id: Id2f8fe05d6b61e96cdd2d41cc43b3d4c3cf39e21
Reviewed-on: https://go-review.googlesource.com/c/tools/+/494095
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Based on feedback in golang/go#60027, tweak the fuzzy symbol scoring
algorithm to much more strongly prefer sequential and exact matches.
Fixesgolang/go#60027
Change-Id: I1c6d019065c4dff4adf2db9e94397a635e13d50f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/493623
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Paul Jolly <paul@myitcv.org.uk>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
Sometimes the unified diff would show the same line removed and added.
That is avoided by this CL.
Fixes: golang/go#59232
Change-Id: Ie6562e3c922b8f0c4319eefdde1913b2d9bc7878
Reviewed-on: https://go-review.googlesource.com/c/tools/+/489695
Run-TryBot: Peter Weinberger <pjw@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
I wasn't thinking of this package as an exported library,
but of course it is, so it should go through proposal review.
Moving to internal until that happens.
Change-Id: Ic8abbe5f6530d5d6201114c1799e26d604f3dd64
Reviewed-on: https://go-review.googlesource.com/c/tools/+/492976
TryBot-Bypass: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
This is the main repo's internal/diff, renamed because
there is a different internal/diff already in x/tools.
Change-Id: I7b6da718e46c9fa23931908b520b9f39c178206b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/491915
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: David Chase <drchase@google.com>
The gopls go.mod file often contains a replace directive, but the
target of the replacement (a parent directory) is not present when the
test is run from the module cache or in an equivalent setting.
To avoid having more configurations to test and maintain, we skip
these tests if the replacement directory does not exist or does not
contain a plausible x/tools go.mod file.
Fixesgolang/go#59841.
Change-Id: Icf86af46899686c3aae410250e6d26ffd11b429a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/489216
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
GNU diff -u prints +0,0 for the special case of deleting into an empty file.
This also adds a test for this case.
What version of Go are you using (go version)? `go version go1.20.3 darwin/amd64`
What operating system and processor architecture are you using? MacOS/amd64
What did you do?
```sh
echo -n "test" >src
touch dst
diff -u src dst
```
What did you expect to see?
```
--- src 2023-04-20 18:04:23
+++ dst 2023-04-20 18:04:23
@@ -1 +0,0 @@
-test
\ No newline at end of file
```
What did you see instead?
internal/diff prints the wrong unified diff, it should be `@@ -1 +0,0 @@` instead of `@@ -1 +1 @@`.
```
--- src
+++ dst
@@ -1 +1 @@
-test
\ No newline at end of file
```
Change-Id: If23b609ada4a45249f1c382ebe8f821dde7aadc0
GitHub-Last-Rev: d08a1ba959
GitHub-Pull-Request: golang/tools#436
Reviewed-on: https://go-review.googlesource.com/c/tools/+/487175
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Detecting the go version does not require build flags,
and should not depend on the value of GO111MODULE.
For golang/go#59841.
Change-Id: I74255953985e6d2ad01095eea2b6d72f7d280e3f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/489215
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Experiments in CL 488655 suggest that the android builders are capable
of running tests that require 'go build' but do not have enough disk
resources to run gopls regression tests. It seems reasonable to always
assume that mobile platforms are (by definition) resource-constrained.
Change-Id: If91627b681050bb2e53d03a86ce609565ae3dc6f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/488815
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
A follow-up change will cause it to write to
the file system.
Change-Id: I1c3b04af1d0f7b8e223d96b4bc9d1399b610f6ee
Reviewed-on: https://go-review.googlesource.com/c/tools/+/488595
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Alan Donovan <adonovan@google.com>
Several non-short tests were missing needed testenv checks.
gopls/internal/lsp/cmd in particular has a weird failure mode.
(I suspect that there is still a missing error check somewhere in that
test.)
For golang/go#58141.
Change-Id: I13d7d19545d46a681a12635eb357617a0fbbd9ab
Reviewed-on: https://go-review.googlesource.com/c/tools/+/486935
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Commit-Queue: Bryan Mills <bcmills@google.com>
testenv.HasExec should not assume that "js" and "ios" are the only
platforms that can't exec. Instead of hard-coding a list of GOOS,
hard-code only the ones known to work and probe the remainder by
trying to re-exec the test binary.
Similarly, in testenv_notunix, negate the list of unix platforms
instead of hard-coding the non-unix ones.
Add missing calls to testenv functions were needed.
For golang/go#58141.
Updates golang/go#59718.
Change-Id: I0114e0bfb6d091e84b325d7f9bb0896da22482be
Reviewed-on: https://go-review.googlesource.com/c/tools/+/486315
Reviewed-by: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Commit-Queue: Bryan Mills <bcmills@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
If ctx is done, that means (by definition) that the caller is no
longer interested in the output of the command. Since 'go list'
output can be quite long, we shouldn't waste time copying it
if we no longer care about it.
If the command doesn't shut down on its own in response to the initial
Interrupt signal, or if it triggers a bug in the kernel leading to a
deadlock, or I/O is otherwise extremely slow (perhaps due to
swapping), the savings from this earlier shutdown could be
significant. Even if not, the impact (or lack thereof) of this change
could tell us more about what's going on.
For golang/go#54461.
Change-Id: I0df067cb77496dacd29497f0995da6fc1a4a37f1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/484741
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Commit-Queue: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
As suggested by bcmills, this change increases the time
between SIGINT and SIGKILL to 5s (was 1s), and also
suppresses the process dump if SIGKILL returned
"already done".
Updates golang/go#57999
Updates golang/go#54461
Change-Id: Ie0e55a69d3bbfb4224e5f4ea272c7c2f3210e342
Reviewed-on: https://go-review.googlesource.com/c/tools/+/483215
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This will remove the flag analysisinternal.DiagnoseFuzzTests created during golang/go#50198.Malformed fuzz target check will be enabled for cmd/vet.
For golang/go#46218
Change-Id: I5cc8d685a57060f8dd84c1957f0d296a6205ddb6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/471295
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Nooras Saba <saba@golang.org>
Benchmarking demonstrated clearly that gopls must memoize active
packages, to efficiently handle repeated requests related to open files
(e.g. code lens, completion, semantic tokens).
In doing so, gopls is effectively pinning the import graph of these open
packages. However, when those packages changed, this import graph was not
being reused. Furthermore when multiple open packages shared packages in
their import graph, there was a chance that gopls may pin multiple copies
of those packages, if the open packages were type-checked in separate
batches.
This change introduces a new optimization which manages a shared import
graph to be re-used across snapshots. Before performing any
type-checking we re-evaluate this shared import graph. This is purely an
optimization, and is not necessary for correctness. As such, the
feature is guarded behind a compile-time constant, so that it may easily
be disabled for debugging. The plan is to have several such constants.
For golang/go#57987
Change-Id: Ica654ffc8f1e5f39bcab7000c0839ece22e20ab2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/479015
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Now that we do not need a static importMap for importing, and do not
need to eagerly parse or load export data, we can evaluate imported
packages lazily during type-checking, thereby avoiding importing
packages that will not be used.
This has a mild beneficial impact on benchmarks (because iimporting is
already cheap). The other direction -- avoiding invalidating packages
that are unaffected by changes -- should have a more significant impact.
For golang/go#57987
Change-Id: I894656af9ca8dea286b6be55f83c4b6bffaaf110
Reviewed-on: https://go-review.googlesource.com/c/tools/+/473166
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
In preparation for skipping imports that aren't reached, change the
gcimporter.IImportShallow API to accept an func rather than an explicit
map.
Update call sites to use a trivial wrapper around the new API.
Subsequent CLs will avoid building packages that aren't needed.
Updates golang/go#57987
Change-Id: Iee88e5f42fa353cde3e87447031c3e7485f04ca4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/476436
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Alloc profiles are very useful. Add a -profile.alloc flag to the
tool.Profile helper (and therefore, to gopls).
Change-Id: I0250be5eadb1c644c6d7b6386077362ec97b06d0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/473164
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
This function breaks the encapsulation of token.FileSet
to add a set of token.Files efficiently. All of the files
in the resulting set must be nonoverlapping, though
identical elements are permitted (and deduplicated).
This is a precursor to optimizations in gopls's typechecking phase.
Change-Id: I1b731bca334b55756d7ba26562e70232965ae3fc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/476437
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Alan Donovan <adonovan@google.com>
In this CL type-checked packages are made entirely independent of each
other, and package export data and indexes are stored in a file cache.
As a result, gopls uses significantly less memory, and (with a warm
cache) starts significantly faster. Other benchmarks have regressed
slightly due to the additional I/O and export data loading, but not
significantly so, and we have some ideas for how to further narrow or
even close the performance gap.
In the benchmarks below, based on the x/tools repository, we can see
that in-use memory was reduced by 88%, and startup time with a warm
cache by 65% (this is the best case where nothing has changed). Other
benchmarks regressed by 10-50%, much of which can be addressed by
improvements to the objectpath package (golang/go#51017), and by making
package data serialization asynchronous to type-checking.
Notably, we observe larger regressions in implementations, references,
and rename because the index implementations (by Alan Donovan) preceded
this change to type-checking, and so these benchmark statistics compare
in-memory index performance to on-disk index performance. Again, we can
optimize these if necessary by keeping certain index information in
memory, or by decoding more selectively.
name old in_use_bytes new in_use_bytes delta
InitialWorkspaceLoad/tools-12 432M ± 2% 50M ± 2% -88.54% (p=0.000 n=10+10)
name old time/op new time/op delta
StructCompletion/tools-12 27.2ms ± 5% 31.8ms ± 9% +16.99% (p=0.000 n=9+9)
ImportCompletion/tools-12 2.07ms ± 8% 2.21ms ± 6% +6.64% (p=0.004 n=9+9)
SliceCompletion/tools-12 29.0ms ± 5% 32.7ms ± 5% +12.78% (p=0.000 n=10+9)
FuncDeepCompletion/tools-12 39.6ms ± 6% 39.3ms ± 3% ~ (p=0.853 n=10+10)
CompletionFollowingEdit/tools-12 72.7ms ± 7% 108.1ms ± 7% +48.59% (p=0.000 n=9+9)
Definition/tools-12 525µs ± 6% 601µs ± 2% +14.33% (p=0.000 n=9+10)
DidChange/tools-12 6.17ms ± 7% 6.77ms ± 2% +9.64% (p=0.000 n=10+10)
Hover/tools-12 2.11ms ± 5% 2.61ms ± 3% +23.87% (p=0.000 n=10+10)
Implementations/tools-12 4.04ms ± 3% 60.19ms ± 3% +1389.77% (p=0.000 n=9+10)
InitialWorkspaceLoad/tools-12 3.84s ± 4% 1.33s ± 2% -65.47% (p=0.000 n=10+9)
References/tools-12 9.72ms ± 6% 24.28ms ± 6% +149.83% (p=0.000 n=10+10)
Rename/tools-12 121ms ± 8% 168ms ±12% +38.92% (p=0.000 n=10+10)
WorkspaceSymbols/tools-12 14.4ms ± 6% 15.6ms ± 3% +8.76% (p=0.000 n=9+10)
This CL is one step closer to the end* of a long journey to reduce
memory usage and statefulness in gopls, so that it can be more
performant and reliable.
Specifically, this CL implements a new type-checking pass that loads and
stores export data, cross references, serialized diagnostics, and method
set indexes in the file system. Concurrent type-checking passes may
share in-progress work, but after type-checking only active packages are
kept in memory. Consequently, there can be no global relationship
between type-checked packages. The work to break any dependence on
global relationships was done over a long time leading up to this CL.
In order to approach the previous type-checking performance, the
following new optimizations are made:
- the global FileSet is completely removed: repeatedly importing from
export data resulted in a tremendous amount of unnecessary token.File
information, and so FileSets had to be scoped to packages
- files are parsed as a batch and stored in the LRU cache implemented
in the preceding CL
- type-checking is also turned into a batch process, so that
overlapping nodes in the package graph may be shared during large
type-checking operations such as the initial workspace load
This new execution model enables several simplifications:
- We no longer need to trim the AST before type-checking:
TypeCheckMode and ParseExported are gone.
- We no longer need to do careful bookkeeping around parsed files: all
parsing uses the LRU parse cache.
- It is no longer necessary to estimate cache heap usage in debug
information.
There is still much more to do. This new model for gopls's execution
requires significant testing and experimentation. There may be new bugs
in the complicated new algorithms that enable this change, or bugs
related to the new reliance on export data (this may be the first time
export data for packages with type errors is significantly exercised).
There may be new environments where the new execution model does not
have the same beneficial effect. (On the other hand, there may be
some where it has an even more beneficial effect, such as resource
limited environments like dev containers.) There are also a lot of new
opportunities for optimization now that we are no longer tied to a rigid
structure of in-memory data.
*Furthermore, the following planned work is simply not done yet:
- Implement precise pruning based on "deep" hash of imports.
- Rewrite unimported completion, now that we no longer have cached
import paths.
For golang/go#57987
Change-Id: Iedfc16656f79e314be448b892b710b9e63f72551
Reviewed-on: https://go-review.googlesource.com/c/tools/+/466975
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
In gopls we were recently surprised to discover that the
the message obtained by exporting a types.Package, and the
message obtained by importing and reexporting that message,
differ.
(Specifically because the importer calls NewInterfaceType,
which has a shortcut for empty interfaces, and this shortcut
is not used by go/types, so the importer produces a more
compact data structure than go/types. There may be other examples.)
This change documents the longstanding invariant that the encoding
is deterministic for a given types.Package data structure, while
also explicitly disclaiming that the composition of export∘import
is idemptotent.
Change-Id: Ie07efc1046315cd2104e78714d8312ecdf10e817
Reviewed-on: https://go-review.googlesource.com/c/tools/+/472075
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
After refactoring the public API of the bug package, the skip value used
in runtime.Caller became inaccurate for some new APIs. Fix this so that
we can actually see where the bug occurred.
Change-Id: Ie301084187dd9309895b2541c5dfc3b5ec4c63ba
Reviewed-on: https://go-review.googlesource.com/c/tools/+/472135
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This change adds a new encoder type with For method, that
is functionally equivalent to the old For function but avoids
the surprising cost of repeated calls to Scope.Names and
canonicalize (now called namedMethods), both of which
allocate and sorts a slice.
The former canonicalize function was previously applied to
interface types too, but this was costly and unnecessary
as they are already sorted.
The public API change will have to wait for proposal 58668
to be accepted and thus fix issue 51017; this change uses
the linkname hack to expose the function to x/tools to fix a
pressing bug in gopls.
See golang/go#58668
Updates golang/go#51017
Change-Id: Id3e8726517d31371b9376b0e47e320f8b6c5e085
Reviewed-on: https://go-review.googlesource.com/c/tools/+/470679
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
This change adds rudimentary support for @rename
markers, which make an LSP 'rename' request and
assert that the expected file changes are obtained.
We also introduce a separate marker, @renameerr,
for expectations of failure.
The treatment both of changed files and of expected errors
should be used by other markers, for consistency.
Also:
- Env.Mapper returns an error.
- add diff.ApplyBytes and compare.Bytes convenience wrappers.
- simplify fake.ApplyEdits by using ApplyProtocolEdits.
- change ApplyProtocolEdits to use []byte.
Change-Id: Ib64b3d8e12c025f01dd0783b4faee94f082b4895
Reviewed-on: https://go-review.googlesource.com/c/tools/+/466144
Run-TryBot: Alan Donovan <adonovan@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
The imports of the main package are now all of the packages that
were referenced during reading the unified IR. This is similar to
the non-bundled option for iimport.
Without this imports of some packages are not the transitive closure.
Fixesgolang/go#58296
Change-Id: I4c58c99a5538a9f990ca7325baa1637e7141a97e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/465936
Run-TryBot: Tim King <taking@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
This change resolves two interrelated outstanding bugs in handling Named
types and type declarations in importMap().
1) Named types did not visit their Underlying() types.
2) Whether Named types were fully visited was order dependent.
Previously a Named type was not fully visited (methods visited, etc)
when the type declaration was visited by addObj() before the Named
type was visited by addType().
Fixesgolang/go#49469
Change-Id: Ibf9c6d9afd4958d474149edf2749d994199f14b1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/362414
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Tim King <taking@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
FormatVarType re-formats a variable type using syntax, in order to get
accurate presentation of aliases and ellipses. However, as a result it
required typed syntax trees for the type declaration, which may exist in
a distinct package from the current package.
In the near future we may not have typed syntax trees for these
packages. We could type-check on demand, but (1) that could be costly
and (2) it may break qualification using the go/types qualifier.
Instead, perform this operation using a qualifier based on syntax
and metadata, so that we only need a fully parsed file rather than a
fully type-checked package. The resulting expressions may be inaccurate
due to built-ins, "." imported packages, or missing metadata, but that
seems acceptable for the current use-cases of this function, which are
in completion and signature help.
While doing this, add a FormatNodeWithFile helper that allows formatting
a node from a *token.File, rather than *token.FileSet. This can help us
avoid relying on a global fileset. To facilitate this, move the GetLines
helper from internal/gcimporter into a shared tokeninternal package.
For golang/go#57987
Change-Id: I3b8a5256bc2261be8b5175ee360b9336228928ac
Reviewed-on: https://go-review.googlesource.com/c/tools/+/464301
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
Robert Findley
Bryan C. Mills
Alan Donovan
cui fliter
Olivier Mengué
Russ Cox
Peter Weinberger
Ayman Bagabas
Nooras Saba
Tim King