Suggestion for discussion: Use Web Worker in the Editor

knolleary commented 3 years ago

@k-toumura I have spent the last two days looking at nrlint, understanding how you have structured it, how webpack is being used and so on.

I think this is a great starting point. I do have some suggestions I would like to discuss that could have an impact on how the code is structured.

The first suggestion is that I would like to explore changing the linter to run in a Web Worker.

In the current code, the linter runs when anything changes. There are some actions that could cause the nodes:changed event to get trigger lots of times very quickly. That could have a bad performance impact.

My first suggestion was going to be to modify the code to schedule doLint 200ms in the future, but only if it hasn't already been scheduled. That will reduce the number of times it gets called when lots of edit events happen in quick order. Something like:

var linterTimer;
function scheduleLinter() {
   if (!linterTimer) {
      linterTimer = setTimeout(function() {
         linterTimer = null;
         doLint();
      },200)
   }
}

But that still means the linter is running on the main thread of the editor - something that could be solved by using a Web Worker which runs in its own thread.

Running the linter as a Worker

As the Worker runs separately to the editor, it does not have direct access to the DOM or any javascript loaded in the editor (ie the RED.* api). It uses a simple message passing API for the main page to send/receive messages to the worker.

In the editor, when the linter wants to run, it would:

call RED.nodes.createCompleteNodeSet() to get the current flow configuration
call worker.postMessage({cmd: 'lint', flows: currentFlowConfig }) That is all the work the editor has to do.

In the worker, it's onmessage function will get triggered where it will:

check the cmd property and see this is a request to run the linter
it will parse the flow config into the Flow Manipulation API object (some changes needed... see below)
after running the linters, it will use its own postMessage function to pass the result back to the editor
```
postMessage({cmd:'result', result: ...})
```
The editor will receive that in its onmessage handler on the worker object, and then update the UI with the results.

That all sounds very clean and well separated. But it requires a number of changes to how the linter rules are loaded.

Flow Manipulation API

The current editor version of this API calls the various RED.nodes functions to build up its view of the flows. I propose it should be more like the node.js version which is given a flow JSON to begin with.

Note this will only work for reading the flow configuration. It won't be suitable for making changes - but I'm sure we can come up with a suitable way of doing that via the postMessage/onmessage connection between worker and editor.

Creating the worker

Workers are created by calling the Worker constructor with a URL to the .js file that should be loaded for the worker.

I propose we create resources/nrlint-worker.js which is the entry point for the worker.

The nrlint.html that gets loaded in the editor as a plugin can then do the following to get it started:

var myWorker = new Worker('resources/nrlint/nrlint-worker.js')

The next question is how do individual rules get loaded in the worker...

Loading rules in the worker

Each linter rule plugin should have a webpack created bundle in its /resources directory.
Its html file can be something like:
```
RED.plugins.registerPlugin('nrlint-plugin-core', {
   type: 'nrlint-rule',
   worker: 'nrlint/core-worker.js'
}
```
The 'worker' property identifies the name of the bundle that the worker should load
When the main linter code (running in the editor) detects a new linter plugin being registered, it sends a message to the worker like: {"cmd": "load", "url":plugin.worker}
In the worker, when it receives that command, it uses the importScripts function to load that code into the work.

The plugin worker bundle will need to do something to register itself with the worker. I don't have an answer for that yet, although there are a few different ways it could work.

What do you think?

I have prototyped some of this locally - starting a separate worker, passing messages to it, loading worker bundles from other plugins.

I don't yet have a version of the Flow Manipulation API that works just from the flow JSON, otherwise I'd have more working to show you.

What do you think? I'm happy to work on this if you think it is a good direction to go and to get more of a working prototype.

k-toumura commented 3 years ago

Thank you for the detailed review and suggestions for improvement! I'm still learning about Web Workers, but I think this is a good direction to go, as we should avoid the UI response getting worse due to the load on the linter. How about proceed prototype implementation on a separate branch or pull request?

knolleary commented 3 years ago

This has now been merged into master.

node-red / nrlint