From 596a85b56b4961c2b37afc3d71e36e5ae4b90bed Mon Sep 17 00:00:00 2001 From: Ian Cottrell Date: Fri, 31 May 2019 20:58:48 -0400 Subject: [PATCH] internal/lsp: don't queue content changes This updates overlays immeditely, and uses the handle identity change to correctly update the content on demand. Fixes golang/go#32348 Change-Id: I3125a6350cac358b7c0f7dc11f2bd11ae1f41031 Reviewed-on: https://go-review.googlesource.com/c/tools/+/179922 Run-TryBot: Ian Cottrell TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/cache/file.go | 23 ++++++++--------------- internal/lsp/cache/gofile.go | 2 +- internal/lsp/cache/load.go | 5 ----- internal/lsp/cache/session.go | 21 ++++++++++----------- internal/lsp/cache/view.go | 32 +++----------------------------- 5 files changed, 22 insertions(+), 61 deletions(-) diff --git a/internal/lsp/cache/file.go b/internal/lsp/cache/file.go index 424a3229..e37ddcf6 100644 --- a/internal/lsp/cache/file.go +++ b/internal/lsp/cache/file.go @@ -29,6 +29,7 @@ type fileBase struct { fname string view *view + fh source.FileHandle fc *source.FileContent token *token.File } @@ -70,20 +71,12 @@ func (f *fileBase) read(ctx context.Context) { f.fc = &source.FileContent{Error: err} return } - if f.fc != nil { - if len(f.view.contentChanges) == 0 { - return - } - - f.view.mcache.mu.Lock() - err := f.view.applyContentChanges(ctx) - f.view.mcache.mu.Unlock() - - if err != nil { - f.fc = &source.FileContent{Error: err} - return - } + oldFH := f.fh + f.fh = f.view.Session().GetFile(f.URI()) + // do we already have the right contents? + if f.fc != nil && f.fh.Identity() == oldFH.Identity() { + return } - // We don't know the content yet, so read it. - f.fc = f.view.Session().GetFile(f.URI()).Read(ctx) + // update the contents + f.fc = f.fh.Read(ctx) } diff --git a/internal/lsp/cache/gofile.go b/internal/lsp/cache/gofile.go index ac6d5c48..350c84ec 100644 --- a/internal/lsp/cache/gofile.go +++ b/internal/lsp/cache/gofile.go @@ -85,7 +85,7 @@ func (f *goFile) GetPackage(ctx context.Context) source.Package { // isDirty is true if the file needs to be type-checked. // It assumes that the file's view's mutex is held by the caller. func (f *goFile) isDirty() bool { - return f.meta == nil || f.imports == nil || f.token == nil || f.ast == nil || f.pkg == nil || len(f.view.contentChanges) > 0 + return f.meta == nil || f.imports == nil || f.token == nil || f.ast == nil || f.pkg == nil } func (f *goFile) astIsTrimmed() bool { diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index 772c3503..8fdd6f0f 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -13,11 +13,6 @@ func (v *view) loadParseTypecheck(ctx context.Context, f *goFile) ([]packages.Er v.mcache.mu.Lock() defer v.mcache.mu.Unlock() - // Apply any queued-up content changes. - if err := v.applyContentChanges(ctx); err != nil { - return nil, err - } - // If the package for the file has not been invalidated by the application // of the pending changes, there is no need to continue. if !f.isDirty() { diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index 1d71e0e6..6bf962d9 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -65,17 +65,16 @@ func (s *session) NewView(name string, folder span.URI) source.View { ctx := context.Background() backgroundCtx, cancel := context.WithCancel(ctx) v := &view{ - session: s, - id: strconv.FormatInt(index, 10), - baseCtx: ctx, - backgroundCtx: backgroundCtx, - cancel: cancel, - name: name, - env: os.Environ(), - folder: folder, - filesByURI: make(map[span.URI]viewFile), - filesByBase: make(map[string][]viewFile), - contentChanges: make(map[span.URI]func()), + session: s, + id: strconv.FormatInt(index, 10), + baseCtx: ctx, + backgroundCtx: backgroundCtx, + cancel: cancel, + name: name, + env: os.Environ(), + folder: folder, + filesByURI: make(map[span.URI]viewFile), + filesByBase: make(map[string][]viewFile), mcache: &metadataCache{ packages: make(map[string]*metadata), }, diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index e18db6bc..a2e7a813 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -56,13 +56,6 @@ type view struct { filesByURI map[span.URI]viewFile filesByBase map[string][]viewFile - // contentChanges saves the content changes for a given state of the view. - // When type information is requested by the view, all of the dirty changes - // are applied, potentially invalidating some data in the caches. The - // closures in the dirty slice assume that their caller is holding the - // view's mutex. - contentChanges map[span.URI]func() - // mcache caches metadata for the packages of the opened files in a view. mcache *metadataCache @@ -224,33 +217,14 @@ func (v *view) SetContent(ctx context.Context, uri span.URI, content []byte) err v.cancel() v.backgroundCtx, v.cancel = context.WithCancel(v.baseCtx) - v.contentChanges[uri] = func() { - v.session.SetOverlay(uri, content) - } - - return nil -} - -// applyContentChanges applies all of the changed content stored in the view. -// It is assumed that the caller has locked both the view's and the mcache's -// mutexes. -func (v *view) applyContentChanges(ctx context.Context) error { - if ctx.Err() != nil { - return ctx.Err() - } - - v.pcache.mu.Lock() - defer v.pcache.mu.Unlock() - - for uri, change := range v.contentChanges { - change() - delete(v.contentChanges, uri) - } + v.session.SetOverlay(uri, content) return nil } func (f *goFile) invalidate() { + f.view.pcache.mu.Lock() + defer f.view.pcache.mu.Unlock() // TODO(rstambler): Should we recompute these here? f.ast = nil f.token = nil