Run cypher queries on web workers #591

irfannuri · 2017-06-16T13:04:56Z

This PR enables running cypher queries on separate web workers (when available) so that web browser does not freeze and remains responsive. A brief list of facts about the implementation is listed below:

There still exists an enduring connection in main thread to perform regular metadata updates and such needs
When a cypher query is executed from editor (so routedWriteTransaction is called), a new web worker is created and the request is passed to that worker
Each web worker creates a new driver object, session and connection which survive through request lifecycle
Web worker and connection related resources are disposed either with the ordinary termination or cancellation of request.

With the use of web workers, it is possible to run multiple long running queries and still use the browser as seen in the attached screen shot. This was not possible, especially when the running query immediately started streaming results, which caused bolt driver to fully occupy cpu thus making UI non-responsive.

To disable web workers, for a limited time you can do so by :config useCypherThread: false.
This option will probably be removed in the future.

changelog: Use separate thread for running cypher queries

oskarhane

Good work!
I added a few questions for things I don't understand.

oskarhane · 2017-06-26T06:09:31Z

package.json

    "webpack-dashboard": "^0.4.0",
-    "webpack-dev-server": "^2.4.2"
+    "webpack-dev-server": "^2.4.2",
+	"worker-loader": "^0.8.0"


Indentation

oskarhane · 2017-06-26T06:10:25Z

src/shared/services/bolt/bolt.js

+  if (enableWebWorkers && window.Worker) {
+    const id = requestId || v4()
+    /* eslint-disable import/no-webpack-loader-syntax */
+    const BoltWorkerModule = require('worker-loader!./boltWorker.js')


Why not import as a regular module at the top of the file?

oskarhane · 2017-06-26T06:12:02Z

src/shared/services/bolt/bolt.js

+let connectionProperties = null
+let boltWorkerRegister = {}
+let cancellationRegister = {}
+let enableWebWorkers = true


Maybe we should start off by have this as a "experimental" setting in the config pane?
wdyt?

oskarhane · 2017-06-26T06:13:31Z

src/shared/services/bolt/boltMappings.js

  return rows.map((row) => row.map((entry) => (entry && entry.properties) ? entry.properties : entry))
 }
+
+export const applyGraphTypes = (item) => {


I'd like to see this function tested, so we don't introduce regressions if we need to make changes in it in the future.

Definitely. Done!

oskarhane · 2017-06-26T06:14:50Z

src/shared/services/bolt/boltWorker.js

+  }
+}
+
+self.addEventListener('message', onmessage)


What is self?

It is needed by worker loader on the worker script

oskarhane · 2017-06-26T06:17:55Z

src/shared/services/bolt/boltMappings.js

+
+export const applyGraphTypes = (item) => {
+  if (Array.isArray(item)) {
+    return item.map(arrayItem => applyGraphTypes(arrayItem))


Minor thing, but it's easier to read if you do this item.map(applyGraphTypes).

oskarhane · 2017-06-26T06:18:20Z

src/shared/services/bolt/boltMappings.js

+  } else if (item.hasOwnProperty('segments') && item.hasOwnProperty('start') && item.hasOwnProperty('end')) {
+    const start = applyGraphTypes(item.start)
+    const end = applyGraphTypes(item.end)
+    const segments = item.segments.map(segment => applyGraphTypes(segment))


Same as above: item.map(applyGraphTypes).

oskarhane · 2017-06-27T18:34:20Z

src/shared/services/bolt/boltMappings.test.js

+    test('should apply node type', () => {
+      const rawNode = {
+        labels: ['Test'],
+        properties: [],


Please add some properties of different types (integers, string, array, null, object) to make sure they come along.

oskarhane · 2017-06-27T18:36:54Z

src/shared/services/bolt/boltMappings.js

+    return new neo4j.types.Relationship(neo4j.int(item.identity), neo4j.int(item.start), neo4j.int(item.end), item.type, applyGraphTypes(item.properties))
+  } else if (item.hasOwnProperty('low') && item.hasOwnProperty('high') && typeof item.low === 'number' && typeof item.high === 'number') {
+    return neo4j.int(item)
+  } else if (typeof item === 'object') {


null is of type object so this will probably throw.

definitely! done.

oskarhane · 2017-06-27T18:37:48Z

src/shared/services/bolt/boltMappings.test.js

+      expect(typedRelationships[1].end instanceof neo4j.Integer).toEqual(true)
+    })
+
+    test('should apply to custom object properties', () => {


This test tests nodes again, not regular js objects. Please add tests for primitives as well, like bool, null, string, float etc.

oskarhane · 2017-08-08T20:27:39Z

I just rebased this over master and things are looking good.
I removed the config UI and renamed the config option to useCypherThread (we might use service worker in the future, so no need to specify what type of thread we use) and enabled it by default.

There's a jumpy behaviour going from spinner -> result that needs to be addressed before I want to merge this.
Also I want to do some more testing with more web browsers.

oskarhane · 2017-08-10T20:31:13Z

I've now addressed the "double frame insert animation" I was seeing.
I've also tested in major web browsers and had to Promise support in the worker file for it to work in IE 11.
To have the web worker working when the server serving the files is offline I had to make it an "inline worker". I've found no issues with it so far.

I'm approving this to be merged. Anyone else want to have a last check before merging @pe4cey @irfannuri @akollegger?

I'm merging after lunch tomorrow (CEST) unless someone tells me not to 🐎

…ger type

…input object

If not the web server serving the Neo4j Browser have to be online to be able to run queries.

Set web worker file as: `/* eslint-env serviceworker */` instead.

oskarhane reviewed Jun 26, 2017

View reviewed changes

oskarhane reviewed Jun 27, 2017

View reviewed changes

oskarhane force-pushed the run-cypher-on-web-workers branch from 3e48d4c to 90a069f Compare June 29, 2017 09:18

oskarhane force-pushed the run-cypher-on-web-workers branch from 90a069f to b4202cc Compare August 8, 2017 19:58

oskarhane force-pushed the run-cypher-on-web-workers branch from 1eda6b6 to 166034e Compare August 10, 2017 20:26

oskarhane added the changelog label Aug 10, 2017

irfannuri and others added 19 commits August 11, 2017 13:56

Move cypher executions to web workers

20db75c

Finalize web workers properly

77e5334

Avoid every object with high and low props to converted to neo4j Inte…

dfdc1d1

…ger type

Merge the changes on code that were moved to boltConnection file

6ced77b

Disable standard.js webpack-loader rule for web worker loading

fc59dc2

Fix the issues that avoid running queries to be cancelled

92bd281

Remove unnecessary arrow function usage

e661a0f

Introduce a setting to let users enable or disable web workers

ac2de1b

Enhanced tests for apply graph types function and null check for the …

e767a4c

…input object

Import worker loader as a proper import at the top

d765dd8

Update test snapshot

eb5cd2d

Bring back tests for applyGraphTypes

9a85b78

Remove UI for settings

6f8f4ac

Rename config enableWebWorkers -> useCypherThread

caaddeb

Linter changes

e18d0c1

Use inline worker so queries works in offline mode

eaaa2df

If not the web server serving the Neo4j Browser have to be online to be able to run queries.

Polyfill webworker for IE 11

b0a3729

Don’t make self global for the linter

aa9729e

Set web worker file as: `/* eslint-env serviceworker */` instead.

Reset connection credentials on connection close.

04798c4

oskarhane force-pushed the run-cypher-on-web-workers branch from 4275a13 to 04798c4 Compare August 11, 2017 11:57

oskarhane merged commit 84bc32f into neo4j:3.0 Aug 11, 2017

oskarhane deleted the run-cypher-on-web-workers branch August 11, 2017 12:03

Run cypher queries on web workers #591

Run cypher queries on web workers #591

Uh oh!

Conversation

irfannuri commented Jun 16, 2017 • edited by oskarhane Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oskarhane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oskarhane commented Aug 8, 2017

Uh oh!

oskarhane commented Aug 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

irfannuri commented Jun 16, 2017 •

edited by oskarhane

Loading