The GC-based cache has given us a number of problems. First, memory
leaks driven by reference cycles: the Go runtime cannot collect cycles
involving finalizers, which prevents us from writing natural code in
Bind callbacks. If we screw it up, we get a mysterious leak that takes a
long time to track down. Second, the behavior is generally mysterious;
it's hard to predict how long a value lasts, and harder to tell if a
value being live is a bug. Third, we think that it may be interacting
poorly with the GC, resulting in unnecessary memory usage.
The structure of the values we put in the cache is not actually that
complicated -- there are only 5 significant types: parse, typecheck,
analyze, parse mod, and analyze mod. Managing them manually should not
be conceptually difficult, and in fact we already do most of the work
in (*snapshot).clone.
In this CL the cache adds the concept of "generations", which function
as reference counts on cache entries. Entries are still global and
shared across generations, but will be explicitly deleted once no
generations refer to them. The idea is that each snapshot is a new
generation, and can inherit entries from the previous snapshot or leave
them behind to be deleted.
One obvious risk of this scheme is that we'll leave dangling references
to values without actually inheriting them across generations. To
prevent that, getting a value requires passing in the generation at
which it's being read, and an error will be returned if that generation
is dead.
Change-Id: I4b30891efd7be4e10f2b84f4c067b0dee43dcf9c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/242838
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Due to the runtime's inability to collect cycles involving finalizers,
we can't close over handles in memoize.Functions without causing memory
leaks. Up until now we've dealt with that by closing over all the bits
of the snapshot that we want, but it distorts the design of all the code
used in the Functions.
We can solve the problem another way: instead of closing over the
snapshot/view, we can force the caller to pass it in. This is somewhat
scary: there is no requirement that the argument matches the data that
we're working with. But the reality is that this is not a new problem:
the Function used to calculate a cache value is not necessarily the one
that the caller expects. As long as the cache key fully identifies all
the inputs to the Function, the output should be correct. And since the
caller used the snapshot/view to calculate that cache key, it should
always be safe to pass in that snapshot/view. If it's not, then we
already had a bug.
The Arg type in memoize is clumsy, but I thought it would be nice to
have at least a little bit of type safety. I'm open to suggestions.
Change-Id: I23f546638b0c66a4698620a986949087211f4762
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244019
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Calculate and display very crude memory usage statistics. This is
complicated by various levels of sharing and indirection, so the numbers
should be taken with *large* grains of salt and interpreted mostly by
experts.
Still, the results are interesting and helpful.
Change-Id: Ia9aff974c7d5fddd63df0cfd5cecc08ead33cf84
Reviewed-on: https://go-review.googlesource.com/c/tools/+/236163
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
We seem to be leaking cache entries. A simple status page will help us
confirm that.
Change-Id: I485bfff6ebfb5d30655554487583e15a3f49f9a4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/217597
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Late into CL 206879 I started nulling out a handle's function when the
handle finished running. That invalidated a previous assumption that the
field was immutable. Fix the assumption, and since the case of having
multiple computations in flight is at least a little bit possible, try
harder to avoid duplicate work.
Fixesgolang/go#35995.
Change-Id: Ib5e3640f931f95e35748f28f5a82cf75585b305d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/210077
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
If a user is typing fast, they will quickly invalidate many snapshots.
We don't want to stack up a bunch of stale type check and analysis
operations, so we should propagate cancellation through the cache.
Handles are long-lived, so we may cancel an operation only to
restart it again later. Also, there may be multiple operations waiting on
the same computation, and just because one is cancelled doesn't mean we
should necessarily stop. The easiest way to support all that was to add
an explicit state to each handle, and track the number of waiters.
See the code for more details on Handle life cycles.
As far as I can tell, the rest of gopls is prepared for this behavior.
I added an explicit check to the type checking code, where I was worried
it might get overly confused. But long-term it would probably be good to
return an error from Get.
Change-Id: I3ea6e141b52b94300a41248d3f2e039b023709d0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/206879
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This is to remove the confusion around having only handles that have had Get
called pin the value into memory.
Instead now there is a single handle per key, and it is the handle that is
weakly held not the value.
Change-Id: I9e813a0dfe2adf4cb651af9b5cfc8878fa71c041
Reviewed-on: https://go-review.googlesource.com/c/tools/+/186839
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
For all uses inside the lsp we use the detatch logic instead
For tests we build it in the test harness instead
This is in preparation for things on the context becomming important
Change-Id: I7e6910e0d3581b82abbeeb09f9c22a99efb73142
Reviewed-on: https://go-review.googlesource.com/c/tools/+/185677
Run-TryBot: Ian Cottrell <iancottrell@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This adds the ability to tie a background context to the context that created it
in traces, and also cleans up and annotates the context used in type checking.
This gives us detailed connected traces of all the type checking and parsing
logic.
Change-Id: I32721220a50ecb9b4404a4e9354343389d7a5219
Reviewed-on: https://go-review.googlesource.com/c/tools/+/183757
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change is more of an exercise for myself to better understand the
implementation of the memoize package. It adds detailed documentation
for the get function in particular.
I also modified the tests to use a table-driven test format. I'm not
certain if this was the right approach (in case we want to add a
different type of test case in the future), but for now, it seems to
work fine.
Change-Id: I191a3b65af230e0af54b221c9f671582adec6c79
Reviewed-on: https://go-review.googlesource.com/c/tools/+/181685
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
This library holds onto results with a weak reference, and guarantees that for
as long as
a result has not been garbage collected it will return the same result for the
same key.
Change-Id: I4a4528f31bf8bbf18809cbffe95dc93e05d769fe
Reviewed-on: https://go-review.googlesource.com/c/tools/+/180845
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
