don't send plugin metadata over JSON-RPC #6233

akosyakov · 2019-09-20T11:02:38Z

What it does

fix plugin system does not scale over JSON-RPC #6228:
- don't send plugin metadata over JSON-RPC, see why and how here
- don't send debugger contributions from the plugin host process to the frontend, but instead initially them statically from plugin metadata, see
update the changelog with a breaking change

How to test

remove native extension from the browse example and rebuild:
- textmate grammar
- debug-nodejs
- java
- java-debug
install 57 VS Code extensions in plugins folder:
https://drive.google.com/file/d/1tuOsorMVOKi_twL9oAuZxgrD7-VUwNsj/view?usp=sharing
- could not come up with a command to pull it from the gdrive in the terminal, if someone knows how to, please share
use system proxy to emulate slow connection, e.g. Charles proxy
- you cannot do it in Chrome, since it cannot throttle web sockets
measure how long does it takes to activate all plugins before and after
observe whether you get yellow status bar and ws frames hangs for 20-30 seconds after and before
play with unstable connections to confirm that plugins get reloaded/unloaded on the reconnection

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

packages/plugin-ext/src/common/plugin-api-rpc.ts

packages/plugin-ext/src/hosted/browser/hosted-plugin.ts

benoitf · 2019-09-20T11:08:02Z

packages/plugin-ext/src/common/plugin-protocol.ts

-    getDeployedFrontendMetadata(): Promise<PluginMetadata[]>;
-    getDeployedBackendMetadata(): Promise<PluginMetadata[]>;
+    getDeployedPlugins(): Promise<string[]>;
+    getDeployedPlugin(id: string): Promise<PluginMetadata | undefined>;


IMHO it looks strange to have same method which is not returning the same object
should be getDeployedMetadata for the second

Could you elaborate please? Is it about undefined? Do you mean that a plugin cannot be undeployed in the meantime?

packages/plugin-ext/src/hosted/browser/hosted-plugin.ts

Signed-off-by: Anton Kosiakov <anton.kosyakov@typefox.io>

So they are available eariler and not transfered twice to the frontend. Signed-off-by: Anton Kosiakov <anton.kosyakov@typefox.io>

akosyakov · 2019-09-23T03:19:46Z

@eclipse-theia/plugin-system @svenefftinge Please have a look when you have time. With this PR, plugins loading should not block anymore. I did not have time to measure the startup time against previous state yet, will do later today or tomorrow.

I did 3 important changes:

serve plugin metadata only once to the frontend in the compressed form via an http endpoint, for 57 plugins size goes to 369kb instead of > 5mb
send to the plugin host process a plugin handle instead of the real plugin, the backend during dispatching a message inflate the handle to the real plugin
initialize debugger contributions from metadata, instead of sending it first to the plugin host and then back to the frontend (in this way plugin metadata used to be sent the second time from the backend to the frontend). Besides that it is slow and unnecessary, it was bogus to delay it till the plugin activation. cc @tolusha

svenefftinge · 2019-09-23T06:17:21Z

packages/plugin-ext/src/common/plugin-api-rpc.ts


-    $init(pluginInit: PluginInitData, configStorage: ConfigStorage): PromiseLike<void>;
+    /** initialize the manager, should be called only once */
+    $initiliaze(params: PluginManagerInitializeParams): Promise<void>;


typo: initialize

svenefftinge · 2019-09-23T06:19:35Z

packages/plugin-ext/src/common/plugin-protocol.ts

@@ -619,11 +641,13 @@ export interface PluginDeployerHandler {
 }

 export const HostedPluginServer = Symbol('HostedPluginServer');
+/**
+ * Don't expose plugin metadata via JSON-RPC. It is expensive to load and blocks the underlying connection.
+ * Instead use `/plugins` http endpoint, it serves compressed metadata for the give set of plugins.


typo: .. the given set of

svenefftinge · 2019-09-23T06:22:01Z

packages/plugin-ext/src/hosted/browser/hosted-plugin-watcher.ts

+    }
+
+    reset(): void {
+        this._initialized = new Deferred<void>();


Should we also generate a new id here, just to avoid confusion with previous connections?

I think it will be confusing if the same window will get different id on reconnect, e.g. you see that some plugin was disconnected for some client id, but then it gets immediately started for another client id without first loading it. I think it would be more consistent to see that a plugin was disconnected for such client and then started again for the same client.

benoitf

I'm -1 about using separate connection for metadata.

AFAIK almost all metadata shouldn't be sent to the client. Changing the way on how to send them is not solving the root cause.

akosyakov · 2019-09-23T08:12:09Z

@benoitf please elaborate on your suggestion, do you mean that the plugin model should be redesigned? How?

akosyakov · 2019-09-23T12:30:44Z

I will try to make an alternative version of a PR which relies on permessage-deflate or send compressed metadata over websocket. About permessage-deflate it seems that it is supported by Firefox and Chrome only, users of other browsers will have blocked UI.

@benoitf please let me know what do you think about first point in #6228 (comment) I would be glad to throw away raw package.json from metadata, it is probably will reduce compressed size to less than 200kb.

benoitf · 2019-09-23T12:52:09Z

@akosyakov less than 200MB ?

akosyakov · 2019-09-23T13:03:48Z

@benoitf sorry, typo 😬 it should be kb

benoitf · 2019-09-23T13:04:24Z

ok, better, thx

akosyakov commented Sep 20, 2019

View reviewed changes

packages/plugin-ext/src/common/plugin-api-rpc.ts Outdated Show resolved Hide resolved

akosyakov commented Sep 20, 2019

View reviewed changes

packages/plugin-ext/src/hosted/browser/hosted-plugin.ts Outdated Show resolved Hide resolved

benoitf reviewed Sep 20, 2019

View reviewed changes

akosyakov force-pushed the ak/plugin_perf branch from 13212b0 to 699f8de Compare September 22, 2019 04:14

akosyakov changed the title ~~pull/push plugin metadata one by one~~ don't send plugin metadata over web socket Sep 22, 2019

akosyakov force-pushed the ak/plugin_perf branch from 699f8de to 83f7d52 Compare September 22, 2019 04:18

akosyakov changed the title ~~don't send plugin metadata over web socket~~ don't send plugin metadata over JSON-RPC Sep 22, 2019

akosyakov force-pushed the ak/plugin_perf branch from 83f7d52 to b899158 Compare September 22, 2019 06:01

akosyakov commented Sep 22, 2019

View reviewed changes

packages/plugin-ext/src/hosted/browser/hosted-plugin.ts Outdated Show resolved Hide resolved

akosyakov added 2 commits September 23, 2019 08:05

fix #6228: don't send plugin metadata over json-rpc

b851bb0

Signed-off-by: Anton Kosiakov <anton.kosyakov@typefox.io>

[plugin] install debugger contribution statically

c29d1ce

So they are available eariler and not transfered twice to the frontend. Signed-off-by: Anton Kosiakov <anton.kosyakov@typefox.io>

akosyakov force-pushed the ak/plugin_perf branch from b899158 to c29d1ce Compare September 23, 2019 03:05

svenefftinge reviewed Sep 23, 2019

View reviewed changes

benoitf requested changes Sep 23, 2019

View reviewed changes

This was referenced Sep 24, 2019

don't block web socket with many plugins #6252

Merged

improve plugins loading #6267

Closed

svenefftinge closed this in #6252 Sep 27, 2019

akosyakov deleted the ak/plugin_perf branch October 4, 2019 07:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

don't send plugin metadata over JSON-RPC #6233

don't send plugin metadata over JSON-RPC #6233

akosyakov commented Sep 20, 2019 •

edited

Loading

benoitf Sep 20, 2019

akosyakov Sep 20, 2019

akosyakov commented Sep 23, 2019 •

edited

Loading

svenefftinge Sep 23, 2019

svenefftinge Sep 23, 2019

svenefftinge Sep 23, 2019

akosyakov Sep 23, 2019

benoitf left a comment

akosyakov commented Sep 23, 2019

akosyakov commented Sep 23, 2019 •

edited

Loading

benoitf commented Sep 23, 2019

akosyakov commented Sep 23, 2019 •

edited

Loading

benoitf commented Sep 23, 2019

don't send plugin metadata over JSON-RPC #6233

don't send plugin metadata over JSON-RPC #6233

Conversation

akosyakov commented Sep 20, 2019 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

benoitf Sep 20, 2019

Choose a reason for hiding this comment

akosyakov Sep 20, 2019

Choose a reason for hiding this comment

akosyakov commented Sep 23, 2019 • edited Loading

svenefftinge Sep 23, 2019

Choose a reason for hiding this comment

svenefftinge Sep 23, 2019

Choose a reason for hiding this comment

svenefftinge Sep 23, 2019

Choose a reason for hiding this comment

akosyakov Sep 23, 2019

Choose a reason for hiding this comment

benoitf left a comment

Choose a reason for hiding this comment

akosyakov commented Sep 23, 2019

akosyakov commented Sep 23, 2019 • edited Loading

benoitf commented Sep 23, 2019

akosyakov commented Sep 23, 2019 • edited Loading

benoitf commented Sep 23, 2019

akosyakov commented Sep 20, 2019 •

edited

Loading

akosyakov commented Sep 23, 2019 •

edited

Loading

akosyakov commented Sep 23, 2019 •

edited

Loading

akosyakov commented Sep 23, 2019 •

edited

Loading