Web worker for amp-bind by dreamofabear · Pull Request #7436 · ampproject/amphtml

dreamofabear · 2017-02-08T20:33:12Z

Fixes #6910, partial for #6199.

Implements a Worker in a new entry point compiled to dist/ww.js
Adds AmpWorker class that wraps the Worker with a Promise-based API
Use in amp-bind to parse and evaluate expressions on background thread
Flagged under new "web-worker" experiment

dreamofabear · 2017-02-09T00:39:39Z

/to @cramforce @jridgewell /cc @kmh287

PTAL, working on tests now.

cramforce

Just a few comments. This is exciting!

cramforce · 2017-02-09T03:25:48Z

extensions/amp-bind/0.1/bind-impl.js


-      this.evaluator_ = new BindEvaluator();
-      const parseErrors = this.evaluator_.setBindings(bindings);
+      // Parse on web worker if experiment is enabled.


Are you making this an experiment to not disrupt your other development, for easy benchmarking, or is there a different reason?

Mostly to reduce risk of breakages as we start talking to publishers (e.g. #7329). I can remove this shortly after verifying correctness in prod.

cramforce · 2017-02-09T03:29:49Z

extensions/amp-bind/0.1/bind-impl.js

+          const {property, expressionString, previousResult} =
+              boundProperty;
+
+          // Rewrite attribute value if necessary.


Definitely fine for now. The URL API may be exposed to workers.

The problem is that parseUrl uses document.createElement, which IIUC is unavailable to workers.

To workaround, we could either change this implementation to not use DOM or even use Worker#postMessage to delegate this to main page. Thoughts?

He's talking about the URL API.

Ohh I see, thanks. Add a TODO here.

cramforce · 2017-02-09T03:36:55Z

src/web-worker/amp-worker.js

+   */
+  sendMessage_(method, args) {
+    const promise = new Promise((resolve, reject) => {
+      if (!this.messages_[method]) {


This works, but it might be simpler to have a map of outstanding messages with a simple running index that you increment for each message.

Fair, done.

cramforce · 2017-02-09T03:38:42Z

src/web-worker/amp-worker.js

+      }
+    }
+    if (empty) {
+      delete this.messages_[method];


As explained above, this is simpler with a running index and one map independent of method name.

cramforce · 2017-02-09T03:41:32Z

src/web-worker/web-worker.js

+
+  let returnValue;
+
+  switch (method) {


Please add TODO and file issue to add error reporting to this.

We can slightly generify the code in src/service-worker/error-reporting.js

Done. Filed #7453.

cramforce · 2017-02-09T03:42:32Z

src/web-worker/web-worker.js

+      } else {
+        throw new Error(`${method}: BindEvaluator is not initialized.`);
+      }
+      break;


Throw on default case.

…fills into worker

dreamofabear

Thanks for the quick review.

dreamofabear · 2017-02-09T15:59:33Z

extensions/amp-bind/0.1/bind-impl.js


-      this.evaluator_ = new BindEvaluator();
-      const parseErrors = this.evaluator_.setBindings(bindings);
+      // Parse on web worker if experiment is enabled.


Mostly to reduce risk of breakages as we start talking to publishers (e.g. #7329). I can remove this shortly after verifying correctness in prod.

dreamofabear · 2017-02-09T16:02:35Z

src/web-worker/amp-worker.js

+   */
+  sendMessage_(method, args) {
+    const promise = new Promise((resolve, reject) => {
+      if (!this.messages_[method]) {


Fair, done.

dreamofabear · 2017-02-09T16:36:15Z

src/web-worker/amp-worker.js

+      }
+    }
+    if (empty) {
+      delete this.messages_[method];


dreamofabear · 2017-02-09T16:36:49Z

src/web-worker/web-worker.js

+
+  let returnValue;
+
+  switch (method) {


Done. Filed #7453.

dreamofabear · 2017-02-09T16:36:55Z

src/web-worker/web-worker.js

+      } else {
+        throw new Error(`${method}: BindEvaluator is not initialized.`);
+      }
+      break;


dreamofabear · 2017-02-09T17:25:00Z

extensions/amp-bind/0.1/bind-impl.js

+          const {property, expressionString, previousResult} =
+              boundProperty;
+
+          // Rewrite attribute value if necessary.


The problem is that parseUrl uses document.createElement, which IIUC is unavailable to workers.

To workaround, we could either change this implementation to not use DOM or even use Worker#postMessage to delegate this to main page. Thoughts?

jridgewell · 2017-02-09T18:49:33Z

extensions/amp-bind/0.1/bind-impl.js

+        return invokeWebWorker(this.win_, 'bind.initialize', [bindings]);
+      } else {
+        this.evaluator_ = new BindEvaluator();
+        const parseErrors = this.evaluator_.setBindings(bindings);


No need to wrap this in a promise instance, just return parseErrors.

jridgewell · 2017-02-09T18:50:21Z

extensions/amp-bind/0.1/bind-impl.js

      }

-      dev().fine(TAG, `Initialized ${bindings.length} bindings from ` +
-          `${boundElements.length} elements.`);


Not just a worker.

jridgewell · 2017-02-09T18:55:20Z

extensions/amp-bind/0.1/bind-impl.js

+          const {property, expressionString, previousResult} =
+              boundProperty;
+
+          // Rewrite attribute value if necessary.


He's talking about the URL API.

jridgewell · 2017-02-09T19:02:15Z

src/web-worker/amp-worker.js

+    return Promise.reject(`Experiment "${TAG}" is disabled.`);
+  }
+  if (!win.Worker) {
+    return Promise.reject('Worker not supported in window: ' + win);


The + win will generate a useless "[object Window]" string.

jridgewell · 2017-02-09T19:03:36Z

gulpfile.js

+  var opts = Object.assign({}, options);
+
+  return compileJs('./src/web-worker/', 'web-worker.js', './dist/', {
+    toName: 'ww.max.js',


This will require special handling on our servers. /cc @vitaliybl

Good to know, I'll also reach out.

Also CC @erwinmombay: A new JS binary on the way.

jridgewell · 2017-02-09T19:04:48Z

src/web-worker/amp-worker.js

+   * @private
+   */
+  sendMessage_(method, args) {
+    const promise = new Promise((resolve, reject) => {


Nit: return here.

jridgewell · 2017-02-09T19:09:14Z

src/web-worker/amp-worker.js

+    const {method, returnValue, id} =
+        /** @type {FromWorkerMessageDef} */ (event.data);
+
+    const message = this.messages_[id];


I hate this id based on messages.length. Let's use a monotonic AmpWorker#id, increasing on every message. Then do a binary search when we receive a message. (We're already doing O(n) on every message receipt)

Splice completed messages from the array and binary search to find them? Seems more complex and slower, I don't see the benefit.

...hate leads to suffering, suffering leads to the dark side.

😄

We have a never ending array this way...

Only while messages are in flight which shouldn't be an issue for current functionality.

Changed this to use a counter per Malte's comment above.

jridgewell · 2017-02-09T19:09:34Z

src/web-worker/amp-worker.js

+    let empty = true;
+    for (let i = 0; i < this.messages_.length && empty; i++) {
+      if (this.messages_[i] !== undefined) {
+        empty = false;


Break afterwards.

The termination condition contains && empty and @camelburrito hates break.

Should be a while loop, then. 😛

jridgewell · 2017-02-09T19:09:57Z

src/web-worker/amp-worker.js

+      }
+    }
+    if (empty) {
+      this.messages_ = [];


Nit: message.length = 0 will truncate and save the array instance.

jridgewell · 2017-02-09T19:11:47Z

src/web-worker/web-worker.js

+
+  // TODO(choumx): Add error reporting.
+  switch (method) {
+    case 'bind.initialize':


Nit: I dislike massive switch statements. Can we split this these into functions (we can keep the switch, then), or split them into functions and use some sort of map from "method" -> function.

Point taken. With only two cases at the moment, using a <string, Function> map is less readable IMO. Besides, this file will need to be significantly refactored to support other use cases in the future anyways.

I mean something like:

switch (method) { case "whatever": returnValue = whatever(...); break; case "other": returnValue = other(...); break; }

dreamofabear

Thanks for the review.

dreamofabear · 2017-02-09T19:53:27Z

extensions/amp-bind/0.1/bind-impl.js

+        return invokeWebWorker(this.win_, 'bind.initialize', [bindings]);
+      } else {
+        this.evaluator_ = new BindEvaluator();
+        const parseErrors = this.evaluator_.setBindings(bindings);


dreamofabear · 2017-02-09T19:53:45Z

extensions/amp-bind/0.1/bind-impl.js

      }

-      dev().fine(TAG, `Initialized ${bindings.length} bindings from ` +
-          `${boundElements.length} elements.`);


dreamofabear · 2017-02-09T19:54:21Z

extensions/amp-bind/0.1/bind-impl.js

+          const {property, expressionString, previousResult} =
+              boundProperty;
+
+          // Rewrite attribute value if necessary.


Ohh I see, thanks. Add a TODO here.

dreamofabear · 2017-02-09T19:55:53Z

src/web-worker/amp-worker.js

+    return Promise.reject(`Experiment "${TAG}" is disabled.`);
+  }
+  if (!win.Worker) {
+    return Promise.reject('Worker not supported in window: ' + win);


dreamofabear · 2017-02-09T19:56:15Z

src/web-worker/amp-worker.js

+   * @private
+   */
+  sendMessage_(method, args) {
+    const promise = new Promise((resolve, reject) => {


dreamofabear · 2017-02-09T20:00:47Z

src/web-worker/amp-worker.js

+    const {method, returnValue, id} =
+        /** @type {FromWorkerMessageDef} */ (event.data);
+
+    const message = this.messages_[id];


Splice completed messages from the array and binary search to find them? Seems more complex and slower, I don't see the benefit.

...hate leads to suffering, suffering leads to the dark side.

😄

dreamofabear · 2017-02-09T20:01:27Z

src/web-worker/amp-worker.js

+    let empty = true;
+    for (let i = 0; i < this.messages_.length && empty; i++) {
+      if (this.messages_[i] !== undefined) {
+        empty = false;


The termination condition contains && empty and @camelburrito hates break.

dreamofabear · 2017-02-09T20:01:38Z

src/web-worker/amp-worker.js

+      }
+    }
+    if (empty) {
+      this.messages_ = [];


dreamofabear · 2017-02-09T20:07:04Z

src/web-worker/web-worker.js

+
+  // TODO(choumx): Add error reporting.
+  switch (method) {
+    case 'bind.initialize':


Point taken. With only two cases at the moment, using a <string, Function> map is less readable IMO. Besides, this file will need to be significantly refactored to support other use cases in the future anyways.

dreamofabear · 2017-02-09T20:08:55Z

gulpfile.js

+  var opts = Object.assign({}, options);
+
+  return compileJs('./src/web-worker/', 'web-worker.js', './dist/', {
+    toName: 'ww.max.js',


Good to know, I'll also reach out.

jridgewell · 2017-02-09T20:21:02Z

src/web-worker/web-worker.js

+      break;
+
+    case 'bind.evaluate':
+      if (evaluator_) {


Do web workers terminate? If so, evaluator_ could be undefined even if it was initialized previously.

I don't think they do. But we'll find out via error reporting.

As far as I can tell, they only terminate on request.

cramforce · 2017-02-10T01:02:08Z

src/web-worker/amp-worker.js

+     * Array of in-flight messages pending response from worker.
+     * @const @private {!Array<(PendingMessageDef|undefined)>}
+     */
+    this.messages_ = [];


Sorry, missed this earlier: This should be an object. Makes the deletion after messages are done so much easier.

Good idea, done.

cramforce · 2017-02-10T16:02:42Z

src/web-worker/amp-worker.js

+   */
+  sendMessage_(method, args) {
+    return new Promise((resolve, reject) => {
+      this.messages_[this.counter_] = {method, resolve, reject};


While we don't need to be thread safe, this is still dangerous if anything throws an exception.

Something like const messageIndex = this.counter_++ would be more obvious, I think.

jridgewell · 2017-02-11T03:01:41Z

extensions/amp-bind/0.1/bind-impl.js

     */
    this.digestQueuedAfterScan_ = false;

+    /** @const @private {boolean} */


We also need to check that web workers are supported. Might be better to export a function from the amp-worker.js module.

Looks like web workers are available for all browsers we care about, so I don't think we should bother supporting the fallback case. It's still useful to check this in amp-worker.js so we can reject invokeWebWorker with a meaningful error message.

jridgewell · 2017-02-11T03:02:56Z

src/web-worker/amp-worker.js

+
+    /**
+     * Array of in-flight messages pending response from worker.
+     * @const @private {!Object<number, (PendingMessageDef|undefined)>}


No need for the |undefined.

jridgewell · 2017-02-11T03:03:52Z

src/web-worker/amp-worker.js

+  sendMessage_(method, args) {
+    return new Promise((resolve, reject) => {
+      const id = this.counter_++;
+      this.messages_[id] = {method, resolve, reject};


reject isn't used?

May be useful for returning warnings or errors to the caller. Error reporting for the worker is next: #7453

jridgewell · 2017-02-11T03:05:05Z

src/web-worker/web-worker.js

+  let returnValue;
+
+  // TODO(choumx): Add error reporting.
+  switch (method) {


Let's wrap this in a try-catch, then post back an error if something goes wrong.

Good idea. Tracked in #7453.

jridgewell · 2017-02-13T22:19:45Z

test/functional/web-worker/test-amp-worker.js

    fakeWin = {Worker: fakeWorkerClass};

-    toggleExperiment(fakeWin, 'web-worker', true);
+    isExperimentOnStub = sandbox.stub(experiments, 'isExperimentOn');


This is supppppperrrrr icky, and not valid ES6. import * as experiments means experiments is not an mutable object.

Why was this changed?

Test was failing due to a recent change in the implementation of toggleExperiment and figured stubbing is less brittle. Is there no good way to stub exported methods?

Reverted and changed to avoid the source of the original error.

You have to export a setter method from the module:

export function method() {} export function setMethodForTesting(fn) { method = fn; }

Ew. 😄

Thanks for the review.

* initial commit for bind worker * move URL rewriting to bind service, add promise api * clean up, fix lint and type errors * install AmpWorker service * add web-worker experiment * add worker experiment fallback * type check web-worker.js * comments and tweaks * mark postMessage with OK * fix dep check and tests * add tests for amp-worker.js * malte's comments * move typedefs to separate file to avoid browserify from bundling polyfills into worker * fix test and type error * justin's comments * malte's comment * increment counter first * justin's comments * fix unit test * fix unit test in a different way

William Chou added 8 commits February 7, 2017 13:14

initial commit for bind worker

287b011

move URL rewriting to bind service, add promise api

c30ee8d

clean up, fix lint and type errors

ec65e86

install AmpWorker service

3e7c2c3

add web-worker experiment

6d219f3

add worker experiment fallback

70ef902

type check web-worker.js

88acfe7

comments and tweaks

9024036

dreamofabear added the PR: Work in Progress label Feb 8, 2017

William Chou added 2 commits February 8, 2017 15:54

mark postMessage with OK

0525453

fix dep check and tests

ac6d200

dreamofabear force-pushed the bind-worker branch from 8819c6e to ac6d200 Compare February 8, 2017 23:09

dreamofabear requested review from cramforce and jridgewell February 9, 2017 00:39

add tests for amp-worker.js

6db62be

dreamofabear force-pushed the bind-worker branch from 3cff9b2 to 6db62be Compare February 9, 2017 01:50

cramforce requested changes Feb 9, 2017

View reviewed changes

William Chou added 2 commits February 9, 2017 12:08

malte's comments

ea31d40

move typedefs to separate file to avoid browserify from bundling poly…

22fcfd0

…fills into worker

dreamofabear commented Feb 9, 2017

View reviewed changes

fix test and type error

290d318

dreamofabear force-pushed the bind-worker branch from e0a7719 to 290d318 Compare February 9, 2017 19:03

jridgewell requested changes Feb 9, 2017

View reviewed changes

justin's comments

e6b238c

dreamofabear commented Feb 9, 2017

View reviewed changes

jridgewell reviewed Feb 9, 2017

View reviewed changes

cramforce approved these changes Feb 9, 2017

View reviewed changes

dreamofabear removed the PR: Work in Progress label Feb 9, 2017

cramforce reviewed Feb 10, 2017

View reviewed changes

malte's comment

5e1b2a2

cramforce reviewed Feb 10, 2017

View reviewed changes

increment counter first

006fe90

jridgewell reviewed Feb 11, 2017

View reviewed changes

dreamofabear mentioned this pull request Feb 13, 2017

Weekly Status 2017-02-13 #7505

Closed

justin's comments

070a902

jridgewell approved these changes Feb 13, 2017

View reviewed changes

fix unit test

62cdf16

jridgewell requested changes Feb 13, 2017

View reviewed changes

fix unit test in a different way

35768a1

jridgewell approved these changes Feb 13, 2017

View reviewed changes

dreamofabear merged commit e0b62b0 into ampproject:master Feb 13, 2017

dreamofabear deleted the bind-worker branch February 13, 2017 22:59

rsimha mentioned this pull request Jan 18, 2018

Consolidate gulp watch and gulp build #12900

Merged

Conversation

dreamofabear commented Feb 8, 2017

Uh oh!

dreamofabear commented Feb 9, 2017

Uh oh!

cramforce 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!

dreamofabear 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!

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