Whenever a view.importsState value is created, we use
snapshot.goCommandInvocation to extract information used to run the go
command in the imports.ProcessEnv. Before this CL, that process acquired
the current snapshot (whatever it was), and held onto it for the purpose
of sharing a workspace directory. As a consequence all the memory in
that snapshot was pinned for the lifecycle of the importsState, which
can be the entire editing session. This results in a memory leak as
information in the session is invalidated.
Fix this by creating a copy of the workspace directory to be owned by
the importsState.
Also:
- Add some TODOs
- Clean up some stale comments
Fixesgolang/go#53780
Change-Id: I2c55cc26b2d46c9320c6c7cd86b3e24971cd5073
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416874
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 the lifetime of all handles in the store is
determined by reference counting, we no longer need
the generation feature.
The Arg interface, renamed RefCounted, is now optional,
and causes the lifetime of the argument to be extended
for the duration of the Function call. This is important
when the Get(ctx) context is cancelled, causing the
function call to outlive Get: if Get's reference to
the argument was borrowed, it needs to increase the
refcount to prevent premature destruction.
Also:
- add missing snapshot.release() call in
importsState.populateProcessEnv.
- remove the --memoize_panic_on_destroyed flag.
Change-Id: I0b3d37c16f8b3f550bb10120c066b628c3db244b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416076
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
I had hoped to see a reduction in total allocation, but it does not
appear to be significant according to the included crude benchmark.
Nonetheless this is a slight code clarity improvement.
Change-Id: I94a503b377dd1146eb371ff11222a351cb5a43b7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/411655
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
When using go 1.18 with go.work files, don't fake workspaces by creating
a workspace module: just run from the workspace root and pass -workfile.
For golang/go#44696
Change-Id: Iaa1979d26b1ce67c6e18e0bf26546a504069da8c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/382241
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
The Acquire method is nearly instantaneous; it only potentially blocks
on a small, constant sequence of cache misses, so there is no need to
avoid blocking in it when a Context is cancelled.
An early return when the passed-in Context is canceled was added in CL
242838 to avoid incrementing the Generation's WaitGroup after its
destruction has begun; however, that early return also bypasses the
WaitGroup accounting that blocks Destroy while the generation is still
in use. Instead, we need the invariant that Acquire is not called in
the first place after Destroy, which we can ensure by nilling out
the View's snapshot when we begin destroying it.
I was not able to reproduce golang/go#48774 locally, but I believe
that this CL will fix it. (It may, however, expose other races or
deadlocks that may have been masked by the early return, which we can
then fix separately.)
Fixesgolang/go#48774
Change-Id: Iac36fceb06485f849da5ba0250b44b55f937c44b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/367675
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
We can't assume that we're in module mode when we rebuild the imports
cache. (This is a long standing bug, but I suspect it was made more
common by my recent CL that refactored this code.)
Fixesgolang/go#43544.
Change-Id: Ie35c43e6b6e496bd2fbf49cf9bf06c28cf1dab80
Reviewed-on: https://go-review.googlesource.com/c/tools/+/282113
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
In some cases (such as presenting more helpful error messages), we need
to know the number of go.mod files in the workspace--not just the
active go.mod files. Track both known and active mod files separately.
Change-Id: I068f76c2930c90cd0fdf5ce637e5934210880f65
Reviewed-on: https://go-review.googlesource.com/c/tools/+/273047
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Refactor to use goCommandInvocation to get the evironment for
goimports. That takes care of most of the work; all that's left is to
track the go.mod hash so we know when to clear the cache.
Fixesgolang/go#41836.
Change-Id: I614dacad04adfb879ead55e3b8640e57816534cb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/270877
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
When incorporating the gopls.mod file, the invalidation logic for
workspace module information becomes quite complicated. For example:
+ if a modfile changes we should only invalidate if it is in the set of
active modules
+ the set of active modules can be provided by either the filesystem or
gopls.mod
+ if gopls.mod changes, we may gain or lose active modules in the
workspace
+ if gopls.mod is *deleted*, we may need to walk the filesystem to
actually find all active modules
Doing this with only concrete changes to the snapshot proved
prohibitively complex (at least for me), so a new workspace type is
introduced to manage the module state. This new abstraction is
responsible for tracking the set of active modules, the workspace
modfile, and the set of workspace directories. Its invalidation logic is
factored out of snapshot.clone, so that it can be tested and to
alleviate some of the growing complexity of snapshot.clone.
The workspace type is idempotent, allowing it to be shared across
snapshots without needing to use the cache. There is little benefit to
the cache in this case, since workspace module computation should be
infrequent, and the type itself consumes little memory.
This is made possible because the workspace type itself depends only on
file state, and therefore may be invalidated independently of the
snapshot. The new source.FileState interface is used in testing, and so
that the workspace module may be computed based on both the session file
state as well as the snapshot file state.
As a result of this change, the following improvements in functionality
are possible:
+ in the presence of a gopls.mod file, we avoid walking the filesystem
to detect modules. This could be helpful for working in large
monorepos or in GOPATH, when discovering all modules would be
expensive.
+ The set of active modules should always be consistent with the
gopls.mod file, likely fixing some bugs (for example, computing
diagnostics for modfiles that shouldn't be loaded)
For golang/go#41837
Change-Id: I2da888c097748b659ee892ca2d6b3fbe29c1942e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/261237
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Trust: Robert Findley <rfindley@google.com>
Build flags, -mod, and -modfile are all accepted by disjoint go command
verbs. Mixing them together had the effect of forcing gocommand users to
figure out which went where themselves, which was annoying and
error-prone. Add ModFlag and ModFile fields to Invocation and update all
uses to use them.
go/packages has a BuildFlags field that's bad for the same reason. Add
private modFlag and modFile fields that forward to the corresponding
Invocation fields.
imports.ProcessEnv gets the same treatment.
Fixesgolang/go#41826.
Change-Id: If74d19146e9e62930d7c34f40859c27c25566c4e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/263213
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Workspace mode makes more sense as a property of the snapshot, since
it is determined based on the modules in the workspace. Move it to the
snapshot and enable the GOPATH to modules test. The mode switch means
that we may run `go mod` commands before a `go.mod` is on-disk, so add
handling for that case.
Also, remove the code added in CL 258121 to treat packages starting with
a "_/" the same way as command-line arguments--that's not actually
correct because perfectly valid packages can also have a "_/" package
path prefix.
Fixesgolang/go#40340
Change-Id: I35044f5d108983ba00df1359698bf14217caa982
Reviewed-on: https://go-review.googlesource.com/c/tools/+/260078
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Many of the View's fields are for goimports, which is a good sign it
should be extracted into a separate struct. Do so in preparation for
redesigning the lifecycle.
I'm not certain this is the right direction but I don't want to deal
with a zillion merge conflicts as I figure it out. I'll move it back
later if it does turn out to have been a mistake.
No functional changes intended, just moved stuff around.
Change-Id: Ide4c2002133d00f6aaa92d114dae2b2ea3ad18fc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/260557
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Robert Findley
Alan Donovan
Bryan C. Mills
Heschi Kreinick
Rebecca Stambler