Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

go/types: add context to go/types.Config for cancelation #34683

Closed
muirdm opened this issue Oct 3, 2019 · 15 comments
Closed

go/types: add context to go/types.Config for cancelation #34683

muirdm opened this issue Oct 3, 2019 · 15 comments

Comments

@muirdm
Copy link

@muirdm muirdm commented Oct 3, 2019

I propose we add a mechanism to go/types so type checking can be canceled. gopls re-type checks packages in response to user edits, which can easily come in faster than type checking takes to complete. Because type checking is not cancelable, gopls builds up defunct type checking goroutines that eat up CPU and memory.

The most obvious cancelation mechanism would be to add a context.Context field to the types.Config struct. During type checking, the context would be checked intermittently for doneness.

/cc @stamblerre @griesemer

@griesemer
Copy link
Contributor

@griesemer griesemer commented Oct 3, 2019

Seems like a reasonable proposal but I'd rather not go/types become dependent on context.Context.

How about just a function func() (installed in types.Config) that is called intermittently from go/types and which can be used to do whatever you want?

One could even make this function have simple API (signature) that could be used to report progress/and or tracing, if one were so inclined.

Loading

@muirdm
Copy link
Author

@muirdm muirdm commented Oct 3, 2019

If there are useful pieces of metadata or actions beyond cancelation that are worth exposing then your suggestion sounds great to me.

Something like

type Config {
  ...
  Checkpoint func(*CheckerStatus)
}

type CheckerStatus struct {}
func (*CheckerStatus) Cancel() {}
...

If we want the actual context.Canceled error itself could get propagated, we could change Cancel() to take an error, or make the Checkpoint func return an error in lieu of Cancel() altogether.

I defer to you on whether there is other useful stuff that could take advantage of the generic callback. If there isn't other stuff in particular, then another option is adding (*go/types.Checker).Cancel().

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Oct 14, 2019

I'm proposing a simpler solution, more akin to:

diff --git a/src/go/types/api.go b/src/go/types/api.go
index 2a21ad0c53..7ce2274ad2 100644
--- a/src/go/types/api.go
+++ b/src/go/types/api.go
@@ -114,6 +114,13 @@ type Config struct {
        // error found.
        Error func(err error)
 
+       // If notify is != nil, it is called repeatedly during type-checking
+       // with a notification message indicating progress. The frequency or
+       // message is not further specified (the message may just be a ".").
+       // Another use for Notify is to abort type-checking early by raising
+       // a panic.
+       Notify func(fmt.Stringer)
+
        // An importer is used to import packages referred to from
        // import declarations.
        // If the installed importer implements ImporterFrom, the type
diff --git a/src/go/types/decl.go b/src/go/types/decl.go
index a4fb2b81cc..10e2f0b4bd 100644
--- a/src/go/types/decl.go
+++ b/src/go/types/decl.go
@@ -61,6 +61,10 @@ func (check *Checker) objDecl(obj Object, def *Named) {
                }()
        }
 
+       if check.conf.Notify != nil {
+               check.conf.Notify(obj)
+       }
+
        // Checking the declaration of obj means inferring its type
        // (and possibly its value, for constants).
        // An object's type (and thus the object) may be in one of

The installed Notify will cause a specific panic (designed by the client) if type-checking should be aborted. A defer enveloping the invocation of the type-checker will catch that specific panic and with that, type-checking is aborted.

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Oct 14, 2019

cc: @alandonovan for input

Loading

@griesemer griesemer self-assigned this Oct 14, 2019
@griesemer griesemer removed this from the Unplanned milestone Oct 14, 2019
@griesemer griesemer added this to the Go1.14 milestone Oct 14, 2019
@alandonovan
Copy link
Contributor

@alandonovan alandonovan commented Oct 15, 2019

Is this necessary?

The unit of type checking (and thus cancellation of type checking, today) is the package, and all but the largest packages type-check very quickly, in less than a typical keypress interval when editing. I don't know the implementation of gopls, but does it type check the complete source each time, or does it destroy function bodies for functions in files other than those of interest? If it did that optimization, and implemented cancelation at package granularity (and perhaps de-bounced the input event stream?) I imagine it would be fast enough.

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Oct 15, 2019

@alandonovan Good question; I don't know. @muirrn ?

Also, something that comes to mind: It may the import processing where the time goes since importing one package may require type-checking that package's imports and the package itself (though all those things should be cached). But definitively worthwhile looking into time spent in an importer.

Loading

@stamblerre
Copy link

@stamblerre stamblerre commented Oct 15, 2019

gopls does do the optimization with function bodies, and all dependencies are cached. I think that this issue was raised in response to a specific case where a user holds down a key for an extended period of time, causing the behavior @muirrn described.

There are definitely plenty of optimizations that can still be made within gopls, but type-checking does account for a significant fraction of our memory allocations, so it would be helpful if it were cancelable. In the example of a user holding down a key--if type-checking begins before the next key-press is registered, we are unable to cancel it and end up doing unneeded work.

Loading

@muirdm
Copy link
Author

@muirdm muirdm commented Oct 15, 2019

perhaps de-bounced the input event stream?

The main problem is synchronous LSP commands like completion. Editors often issue a completion request after every keypress, and to minimize completion latency gopls begins to type check ASAP. Any debouncing would reduce responsiveness.

Loading

@alandonovan
Copy link
Contributor

@alandonovan alandonovan commented Oct 15, 2019

Understood, but completion needs to type-check at most one function body, plus all the package-level declarations of a package. It should run in well under a millisecond.

Loading

@stamblerre
Copy link

@stamblerre stamblerre commented Oct 15, 2019

In addition to completion and other language features, gopls provides diagnostics (which can include type-checking and analysis) on every keystroke, and this requires full function bodies for every file in a given package (and occasionally reverse dependencies).

With the current gopls caching, we actually explicitly don't cancel type-checking so that we can save the results for later use (see memoize.go). We would definitely have to rethink this if we could cancel type-checking. I still think it would be a net benefit, particularly since we would still be caching dependencies even if we canceled early.

Our concern is mostly CPU and memory usage, rather than speed. I'm currently debugging a memory leak 🙃, so I don't have accurate metrics yet, but once that is fixed, I will provide some more concrete data as justification.

Loading

@alandonovan
Copy link
Contributor

@alandonovan alandonovan commented Oct 15, 2019

By its nature, the results of completion are required with very low latency as they help the user choose what to type, but diagnostics don't make sense until the program is at least reasonably well formed. Issuing diagnostic requests on every keystroke seems likely to lead to cascades of errors, distracting UI updates, wasted CPU cycles, and drained batteries. I don't think it's a strong justification for this feature.

Loading

@stamblerre
Copy link

@stamblerre stamblerre commented Oct 15, 2019

After some offline discussion with @alandonovan, I think our best approach here would be to check the context inside of the calls to (types.Importer).Import. That maybe enough for now, and it would ensure that we don't too much wasted work.

Loading

@alandonovan
Copy link
Contributor

@alandonovan alandonovan commented Oct 15, 2019

BTW, although the old go/loader historically did actual work in the import function, go/packages, for many good reasons, does the work of loading and type checking dependencies of package P before even beginning to type-check P, so the Import function is just a map lookup. But logically, yes, we could profitably break off a cancelled type-checking task after loading only some of the dependencies.

Loading

@muirdm
Copy link
Author

@muirdm muirdm commented Oct 15, 2019

Thanks everyone. Closing as "rejected".

BTW I really like the idea of doing a minor type check with only the containing function and package decls for synchronous commands like completion. That would make completions snappy even on giant packages (assuming the package isn't one giant function...).

Loading

@muirdm muirdm closed this Oct 15, 2019
@gopherbot
Copy link

@gopherbot gopherbot commented Jan 31, 2020

Change https://golang.org/cl/217257 mentions this issue: internal/lsp/cache: add context cancellation check inside importerFunc

Loading

gopherbot pushed a commit to golang/tools that referenced this issue Jan 31, 2020
This change adds a check inside of the types.ImporterFunc to see if the context has been cancelled.

Updates golang/go#34683

Change-Id: I0f12da0f8158ecda0eec00150ed6ff772c2f89c3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/217257
Run-TryBot: Rohan Challa <rohan@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Jan 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants