Bug 422013 - Move javascript plugin into web worker
Summary: Move javascript plugin into web worker
Status: RESOLVED FIXED
Alias: None
Product: Orion (Archived)
Classification: ECD
Component: JS Tools (show other bugs)
Version: 3.0   Edit
Hardware: PC Windows 7
: P3 normal (vote)
Target Milestone: 6.0 M1   Edit
Assignee: Mark Macdonald CLA
QA Contact:
URL:
Whiteboard:
Keywords: helpwanted
Depends on: 423061
Blocks:
  Show dependency tree
 
Reported: 2013-11-18 18:29 EST by Mark Macdonald CLA
Modified: 2014-03-31 12:46 EDT (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Macdonald CLA 2013-11-18 18:29:06 EST
Apparently Web Workers have decent support these days [1]. We should investigate moving the AST-parsing work done in javascriptPlugin.js into a web worker. Workers run on real background threads, and would help us parse large files without hanging the main thread.

[1] http://caniuse.com/#feat=webworkers
Comment 1 Mark Macdonald CLA 2013-11-18 18:32:20 EST
More thoughts: after constructing the AST, it will still need to be structured-cloned between plugins and the core by the service registry -- which is potentially expensive. 

In fact, using a Worker might add an extra structured clone, as the AST would have to travel from Worker -> plugin -> serviceRegistry. So the net effect could even be negative. 

I'd like to see a prototype of this with some performance measurements on large ASTs.
Comment 3 Mark Macdonald CLA 2013-12-13 18:38:11 EST
I tried using transferables, but I found myself recreating the same problem between javascriptPlugin and Worker that we used to have between the Orion core and javascriptPlugin: we inevitably have to clone or deserialize the AST data structure on the plugin's main thread, which hangs the entire application.

But, it occurred to me that we could move basically all of javascriptPlugin inside a Worker, with just a small proxy running in the plugin's Window to manage communication with the PluginProvider.

This would ensure that the AST never crosses a frame/worker boundary at all. Instead we would clone only the inputs and outputs of service calls, and those objects tend to be much smaller than a full-blown AST. (For example, I tested with a 78000-line JavaScript file. The buffer text returned by getText() was 2.8MB, but its AST model was ~56MB, almost 20x larger.)

Moreover browsers seem to be much faster at cloning strings like the buffer text than they are at structured cloning objects. I will investigate this approach next.
Comment 4 Mark Macdonald CLA 2013-12-16 16:37:47 EST
Made charts of some perf measurements taken in Firefox and Chrome:
https://docs.google.com/spreadsheet/ccc?key=0AnWzHGjgCsBVdHVpcDBzcmV4TVQ1U0hVLXU5Vk8xTnc#gid=3

It shows 3 different approaches I tried for posting the AST from Worker to javascriptPlugin:
  i) Stringify
 ii) Stringify -> ArrayBuffer (transferable)
iii) Structured clone

Lower bars are better. Note that the FF and Chrome charts don't use the same scale on the y-axis.
Comment 5 Mark Macdonald CLA 2014-03-13 19:20:28 EDT
OK, I've gone ahead and pushed this to master:
http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=01e4cfa

I've implemented the scheme described in Comment 3:

  [Framework] <--> [javascriptPlugin] <--> [javascriptWorker]
     Window             Window                  Worker

All the former logic of the JS Plugin now lives in javascriptWorker.js, which runs in a Worker. javascriptPlugin.js is now just a proxy that forwards messages between the Orion core ("Framework") and the worker.

The worker still includes 'plugin.js' and registers the same set of services as before -- all the plugin talk simply gets proxied over to the Framework, and vice-versa. NOTE that JS tooling code must be careful now about what globals it uses -- 'window' is not in scope anymore. See [1] for the API we can use.

@skaegi: plugin.js contained a check that, when run in a Worker context, prevented me from proxying plugin-related messages from the middle Window in the diagram above to the Worker. I had to relax this check (I'll explain in detail over email).

[1] https://developer.mozilla.org/en-US/docs/Web/Reference/Functions_and_classes_available_to_workers
Comment 6 Mark Macdonald CLA 2014-03-13 19:26:03 EDT
Results/details:

The proxying adds overhead, namely 1 additional structured clone of every message sent over the Framework<->JSTools bus. But so far it seems to be worth it.

For example, I tested with a 3.3MB / 91K-line file. While it took almost 9 seconds to parse and validate the file, the UI stays responsive the whole time, which was the goal of this bug.

Once the AST is built and cached, stuff like marking occurrences is fairly responsive.

Content assist is more of a problem, since it typically has to build the AST every time it appears due to the buffer being dirtied by typing. There's a long delay between hitting '.' before you see the proposals. This makes it feel like it isn't working at all.

So stuff to think about next is:
-Make parsing faster
-Get UI for showing you that a long-running operation is underway in the editor.
Comment 7 Mark Macdonald CLA 2014-03-14 09:33:59 EDT
I guess the worker is actually called 'javascriptPluginWorker.js'.
Comment 8 Michael Rennie CLA 2014-03-17 15:30:06 EDT
(In reply to Mark Macdonald from comment #7)
> I guess the worker is actually called 'javascriptPluginWorker.js'.

gotta add my two cents here:

1. I don't notice any difference in performance (anecdotal)
2. having the plugin in a worker makes it almost impossible to debug now
Comment 9 Michael Rennie CLA 2014-03-21 11:59:42 EDT
I have to reopen this.

While it may improve performance for extremely large files, it makes my life a living hell. For example, today I wasted 2 hours chasing a problem I though was from our content assist, but was because of the worker.

In my opinion, if you are working with files that are 20K + LOC you are doing something wrong. We should look at reverting this change.
Comment 10 Mark Macdonald CLA 2014-03-21 14:25:57 EDT
I'm not sure how you got into the broken state you were describing over IM, but does unchecking the "use worker" box in javascriptPlugin, and forcing a plugin reload from the Settings > Plugins page, fix the issue in your environment?
Comment 11 Michael Rennie CLA 2014-03-21 14:52:39 EDT
(In reply to Mark Macdonald from comment #10)
> I'm not sure how you got into the broken state you were describing over IM,
> but does unchecking the "use worker" box in javascriptPlugin, and forcing a
> plugin reload from the Settings > Plugins page, fix the issue in your
> environment?

No, because the state I was in would not allow the pref to be set, as soon as I touched it the console would fill with errors (like I pasted in the IMs).
Comment 12 Michael Rennie CLA 2014-03-24 13:30:36 EDT
(In reply to Michael Rennie from comment #11)
> No, because the state I was in would not allow the pref to be set, as soon
> as I touched it the console would fill with errors (like I pasted in the
> IMs).

As soon as you open a JS file in Orion, the tooling immediately stops. 

Steps:

1. open contentAssist.js in host Orion (not a target workspace)
2. check the console and notice:

Error retrieving content assist proposals built-edit.js:3422
Error: plugin activation error
    at Error (native)
    at https://orion.eclipse.org/edit/built-edit.js:2050:293
    at e (https://orion.eclipse.org/edit/built-edit.js:1993:456)
    at MutationObserver.h (https://orion.eclipse.org/edit/built-edit.js:1993:224) 

3. pick any other JS file, notice there is no highlighting, etc.

Once the worker stops the only way to get any of the JS tooling to work again is to reload the whole works.
Comment 13 Michael Rennie CLA 2014-03-24 15:10:26 EDT
(In reply to Michael Rennie from comment #12)
> (In reply to Michael Rennie from comment #11)
> > No, because the state I was in would not allow the pref to be set, as soon
> > as I touched it the console would fill with errors (like I pasted in the
> > IMs).
> 

For now I have de-workered the plug-in:

http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=300b568d1e4b07a6c879a5672fa814bafe213471

We will have to spend more time testing why it fails so horribly in some cases but not all cases. Also it would be good if there were decent dev tools for working with them in the next go-around.
Comment 14 Mark Macdonald CLA 2014-03-31 12:46:31 EDT
http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=2737caa
http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=cce1382

I've worker-ified the plugin again. Going more cautiously this time: the worker is opt-in. You need to enable a hidden flag to get it.

To activate the worker, run this command in the console (or load javascriptPlugin.html in your browser and tick the checkbox).
> localStorage["jstools.worker"] = true;

The next time you load the editor page, the JS tools will be hosted in a worker.

To disable the worker, un-tick the box or run
> localStorage.removeItem("jstools.worker");

In the default case, no worker is used, and the JS tools should run on the UI thread, exactly as they did before the worker saga began.