Skip to content

Commit

Permalink
fix(exthost/#3009): Call '$release*' APIs to clean-up extension host …
Browse files Browse the repository at this point in the history
…cache (#3016)

__Issue:__ There is a memory leak in our completion items, signature help, and codelens via the extension host - may be responsible for the `SIGABRT` described in #3009 (as this can occur when the JS heap is out of memory).

__Defect:__ The extension host adds a layer on top of the language server protocol, to serve as a cache for several language features - this helps performance when resolving items. However, this caching layer relies on the client to notify it when the items are no longer in use, so they can be cleaned up. Onivim was missing this `release` step.

__Fix:__ Bake in the call to `release` in all the relevant subscriptions. This involves picking up the `cacheId` which wasn't wired up in some places (like codelens), setting up the `release*` API, and passing it back on conclusion of the subscription.


To test this fix - I flipped the `Cache.enableDebugLogging` flag in the extension host and initiated several completions. Before this fix, it's obvious the caches were simply growing endlessly:
```
CompletionItem cache size — 1
CompletionItem cache size — 1
CompletionItem cache size — 2
CompletionItem cache size — 2
SignatureHelp cache size — 1
CompletionItem cache size — 3
CompletionItem cache size — 3
CompletionItem cache size — 4
CompletionItem cache size — 4
SignatureHelp cache size — 2
...
CompletionItem cache size — 15
CompletionItem cache size — 15
CompletionItem cache size — 16
CompletionItem cache size — 16
CompletionItem cache size — 17
CompletionItem cache size — 17
SignatureHelp cache size — 8
CompletionItem cache size — 18
CompletionItem cache size — 18
CompletionItem cache size — 19
CompletionItem cache size — 19
```

After this fix, though, we can observe that the cache gets cleaned:
```
CompletionItem cache size — 1
CompletionItem cache size — 1
CompletionItem cache size — 0
CompletionItem cache size — 0
CompletionItem cache size — 1
CompletionItem cache size — 1
CompletionItem cache size — 0
CompletionItem cache size — 0
SignatureHelp cache size — 1
SignatureHelp cache size — 0

...
CompletionItem cache size — 0
CompletionItem cache size — 0
SignatureHelp cache size — 1
SignatureHelp cache size — 0
```

Related #3009 
Related #1058
  • Loading branch information
bryphe authored Jan 20, 2021
1 parent 237320c commit 2885f9f
Show file tree
Hide file tree
Showing 11 changed files with 280 additions and 117 deletions.
1 change: 1 addition & 0 deletions CHANGES_CURRENT.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
- #3008 - SCM: Fix index-out-of-bound exception when rendering diff markers
- #3007 - Extensions: Show 'missing dependency' activation error to user
- #3011 - Vim: Visual Block - Handle 'I' and 'A' in visual block mode (fixes #1633)
- #3016 - Extensions: Fix memory leak in extension host language features (fixes #3009)

### Performance

Expand Down
23 changes: 20 additions & 3 deletions src/Exthost/CodeLens.re
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
[@deriving show]
type t = {
type lens = {
cacheId: option(list(int)),
range: OneBasedRange.t,
command: option(Command.t),
Expand Down Expand Up @@ -33,12 +33,29 @@ let decode = Decode.decode;
let encode = Encode.encode;

module List = {
[@deriving show]
type cacheId = int;

[@deriving show]
type t = {
cacheId: option(cacheId),
lenses: list(lens),
};

let default = {cacheId: None, lenses: []};

module Decode = {
open Oni_Core.Json.Decode;

let simple = list(decode);
let simple = list(decode) |> map(lenses => {cacheId: None, lenses});

let nested = field("lenses", simple);
let nested =
obj(({field, _}) =>
{
lenses: field.required("lenses", list(decode)),
cacheId: field.optional("cacheId", int),
}
);

let decode = one_of([("nested", nested), ("simple", simple)]);
};
Expand Down
43 changes: 34 additions & 9 deletions src/Exthost/Exthost.rei
Original file line number Diff line number Diff line change
Expand Up @@ -100,15 +100,26 @@ module OneBasedRange: {

module CodeLens: {
[@deriving show]
type t = {
type lens = {
cacheId: option(list(int)),
range: OneBasedRange.t,
command: option(Command.t),
};

let decode: Json.decoder(t);
module List: {
[@deriving show]
type cacheId;

module List: {let decode: Json.decoder(list(t));};
[@deriving show]
type t = {
cacheId: option(cacheId),
lenses: list(lens),
};

let default: t;

let decode: Json.decoder(t);
};
};

module Location: {
Expand Down Expand Up @@ -593,9 +604,11 @@ module SignatureHelp: {
let decode: Json.decoder(t);
};

type cacheId;

module Response: {
type t = {
id: int,
cacheId,
signatures: list(Signature.t),
activeSignature: int,
activeParameter: int,
Expand All @@ -606,10 +619,15 @@ module SignatureHelp: {
};

module SuggestResult: {
[@deriving show];

type cacheId;

[@deriving show]
type t = {
completions: list(SuggestItem.t),
isIncomplete: bool,
cacheId: option(cacheId),
};

let empty: t;
Expand Down Expand Up @@ -1758,11 +1776,14 @@ module Request: {
module LanguageFeatures: {
let provideCodeLenses:
(~handle: int, ~resource: Uri.t, Client.t) =>
Lwt.t(option(list(CodeLens.t)));
Lwt.t(option(CodeLens.List.t));

let resolveCodeLens:
(~handle: int, ~codeLens: CodeLens.t, Client.t) =>
Lwt.t(option(CodeLens.t));
(~handle: int, ~codeLens: CodeLens.lens, Client.t) =>
Lwt.t(option(CodeLens.lens));

let releaseCodeLenses:
(~handle: int, ~cacheId: CodeLens.List.cacheId, Client.t) => unit;

let provideCompletionItems:
(
Expand All @@ -1778,6 +1799,9 @@ module Request: {
(~handle: int, ~chainedCacheId: ChainedCacheId.t, Client.t) =>
Lwt.t(SuggestItem.t);

let releaseCompletionItems:
(~handle: int, ~cacheId: SuggestResult.cacheId, Client.t) => unit;

let provideDocumentHighlights:
(
~handle: int,
Expand Down Expand Up @@ -1875,6 +1899,9 @@ module Request: {
) =>
Lwt.t(option(SignatureHelp.Response.t));

let releaseSignatureHelp:
(~handle: int, ~cacheId: SignatureHelp.cacheId, Client.t) => unit;

let provideDocumentFormattingEdits:
(
~handle: int,
Expand Down Expand Up @@ -1904,8 +1931,6 @@ module Request: {
Client.t
) =>
Lwt.t(option(list(Edit.SingleEditOperation.t)));

let releaseSignatureHelp: (~handle: int, ~id: int, Client.t) => unit;
};

module SCM: {
Expand Down
27 changes: 24 additions & 3 deletions src/Exthost/Request.re
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ module LanguageFeatures = {
);
};

let resolveCodeLens = (~handle: int, ~codeLens: CodeLens.t, client) => {
let resolveCodeLens = (~handle: int, ~codeLens: CodeLens.lens, client) => {
let decoder = Json.Decode.(nullable(CodeLens.decode));
Client.request(
~decoder,
Expand All @@ -266,6 +266,17 @@ module LanguageFeatures = {
client,
);
};

let releaseCodeLenses = (~handle: int, ~cacheId, client) => {
Client.notify(
~usesCancellationToken=false,
~rpcName="ExtHostLanguageFeatures",
~method="$releaseCodeLenses",
~args=`List([`Int(handle), `Int(cacheId)]),
client,
);
};

let provideCompletionItems =
(
~handle: int,
Expand Down Expand Up @@ -319,6 +330,16 @@ module LanguageFeatures = {
);
};

let releaseCompletionItems = (~handle: int, ~cacheId, client) => {
Client.notify(
~usesCancellationToken=false,
~rpcName="ExtHostLanguageFeatures",
~method="$releaseCompletionItems",
~args=`List([`Int(handle), `Int(cacheId)]),
client,
);
};

module Internal = {
let provideDefinitionLink =
(~handle, ~resource, ~position, method, client) => {
Expand Down Expand Up @@ -448,15 +469,15 @@ module LanguageFeatures = {
);
};

let releaseSignatureHelp = (~handle, ~id, client) =>
let releaseSignatureHelp = (~handle, ~cacheId, client) =>
Client.notify(
~rpcName="ExtHostLanguageFeatures",
~method="$releaseSignatureHelp",
~args=
`List(
Json.Encode.[
handle |> encode_value(int),
id |> encode_value(int),
cacheId |> encode_value(int),
],
),
client,
Expand Down
7 changes: 5 additions & 2 deletions src/Exthost/SignatureHelp.re
Original file line number Diff line number Diff line change
Expand Up @@ -122,10 +122,13 @@ module Signature = {
);
};

[@deriving show]
type cacheId = int;

module Response = {
[@deriving show]
type t = {
id: int,
cacheId,
signatures: list(Signature.t),
activeSignature: int,
activeParameter: int,
Expand All @@ -135,7 +138,7 @@ module Response = {
Json.Decode.(
obj(({field, _}) =>
{
id: field.required("id", int),
cacheId: field.required("id", int),
signatures: field.required("signatures", list(Signature.decode)),
activeSignature: field.required("activeSignature", int),
activeParameter: field.required("activeParameter", int),
Expand Down
7 changes: 6 additions & 1 deletion src/Exthost/SuggestResult.re
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
open Oni_Core;

[@deriving show]
type cacheId = int;

[@deriving show]
type t = {
completions: list(SuggestItem.t),
isIncomplete: bool,
cacheId: option(cacheId),
};

let empty = {completions: [], isIncomplete: false};
let empty = {completions: [], isIncomplete: false, cacheId: None};

module Dto = {
let decode = {
Expand All @@ -17,6 +21,7 @@ module Dto = {
// https://github.com/onivim/vscode-exthost/blob/50bef147f7bbd250015361a4e3cad3305f65bc27/src/vs/workbench/api/common/extHost.protocol.ts#L1129
completions: field.required("b", list(SuggestItem.Dto.decode)),
isIncomplete: field.withDefault("c", false, bool),
cacheId: field.optional("x", int),
}
)
);
Expand Down
10 changes: 5 additions & 5 deletions src/Feature/LanguageSupport/CodeLens.re
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@ module Log = (

// MODEL

type codeLens = Exthost.CodeLens.t;
type codeLens = Exthost.CodeLens.lens;

let lineNumber = (lens: Exthost.CodeLens.t) =>
let lineNumber = (lens: Exthost.CodeLens.lens) =>
Exthost.OneBasedRange.(lens.range.startLineNumber - 1);

let textFromExthost = (lens: Exthost.CodeLens.t) => {
let textFromExthost = (lens: Exthost.CodeLens.lens) => {
Exthost.Command.(
lens.command
|> OptionEx.flatMap(command => command.label)
Expand All @@ -21,7 +21,7 @@ let textFromExthost = (lens: Exthost.CodeLens.t) => {
);
};

let text = (lens: Exthost.CodeLens.t) => textFromExthost(lens);
let text = (lens: Exthost.CodeLens.lens) => textFromExthost(lens);

type provider = {
handle: int,
Expand Down Expand Up @@ -55,7 +55,7 @@ type msg =
bufferId: int,
startLine: EditorCoreTypes.LineNumber.t,
stopLine: EditorCoreTypes.LineNumber.t,
lenses: list(Exthost.CodeLens.t),
lenses: list(Exthost.CodeLens.lens),
});

let register = (~handle: int, ~selector, ~maybeEventHandle, model) => {
Expand Down
2 changes: 1 addition & 1 deletion src/Feature/LanguageSupport/Feature_LanguageSupport.rei
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ module Msg: {
};

module CodeLens: {
type t = Exthost.CodeLens.t;
type t = Exthost.CodeLens.lens;

let lineNumber: t => int;
let text: t => string;
Expand Down
Loading

0 comments on commit 2885f9f

Please sign in to comment.