Two workers & worker termination #198

jakearchibald · 2018-10-13T15:55:13Z

Building on the ideas in #192, but ensuring we only have two workers on the go.

This involved a lot of refactoring, so I'm really sorry that this PR ballooned. Free free to throw it back at me (if you've got a better way of doing it).

I'll leave some comments to try and make things easier.

@surma it doesn't look like free() is working correctly for mozjpeg. Could you take a look? I updated example.html to show the issue. I was getting "out of bounds" errors, but now it just seems to screw up the second image.

jakearchibald · 2018-10-13T16:00:02Z

I changed the shape of the encoders/preprocessors. encoder-meta.ts contains stuff like default options, types, basically things we're happy being on the main thread. encoder.ts becomes the wasm stuff.

jakearchibald · 2018-10-13T16:03:55Z

src/codecs/imagequant/processor.ts

+import wasmUrl from '../../../codecs/imagequant/imagequant.wasm';
+import { QuantizeOptions } from './processor-meta';
+
+let emscriptenModule: Promise<QuantizerModule>;


I changed how we do these types of files a bit. Instead of having a class, I just treat it as a module. The emscripten module is initialised on first use.

jakearchibald · 2018-10-13T16:06:01Z

src/codecs/processor-worker.ts

@@ -0,0 +1,41 @@
+import { expose } from 'comlink';


This is the worker. It exposes a method for everything we workerise, but it loads the bulk of the code lazily.

I like this a lot better than the per-encoder approach.

This is great for now. Just for documentation purposes: You said in person we’d probably wanna explore some form of code generation or something to make adding new codecs a bit easier in the future.

jakearchibald · 2018-10-13T16:07:46Z

src/codecs/processor.ts

@@ -0,0 +1,194 @@
+import { proxy } from 'comlink';


This is the main-thread part of processor-worker. It exposes all the image processing methods, and cancels any pending jobs.

surma · 2018-10-13T16:58:17Z

Not to go all “It works on my machine” on you, but it works on my machine? I.e. your modified example.html runs without any errors for me.

jakearchibald · 2018-10-13T19:17:10Z

Huh, does the 2nd image not look all fucked? I guess I'll show you next week.

…

On Sat, 13 Oct 2018, 17:58 Surma ***@***.*** wrote: Not to go all “It works on my machine” on you, but it works on my machine? I.e. your modified example.html runs without any errors for me. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#198 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFtmvUOcXsEX0A2wdJMGSAKz53g057gks5ukhupgaJpZM4XasTL> .

developit · 2018-10-13T17:22:03Z

codecs/mozjpeg_enc/mozjpeg_enc.d.ts

@@ -1,4 +1,4 @@
-import { EncodeOptions } from '../../src/codecs/mozjpeg/encoder';
+import { EncodeOptions } from '../../src/codecs/mozjpeg/encoder-meta';


ah good call splitting out the metadata, that helps resolve some oddities with worker/noworker module loading I was lamenting.

developit · 2018-10-15T12:24:40Z

src/codecs/imagequant/processor.ts

+        // prop solves this for now.
+        // See: https://github.com/kripken/emscripten/blob/incoming/src/postamble.js#L129
+        // TODO(surma@): File a bug with Emscripten on this.
+        delete (m as any).then;


I know this isn't part of the PR, but could we change this so it doesn't require the delete? Looking at the source, the thennable never fails, so we could just do:

m.then(resolve)

Also I bet we could extract the initialization into a helper method:

// initWasmModule(imagequant, wasmUrl) function initWasmModule(fn, wasmUrl) { return new Promise((resolve) => { const m = fn({ // Just to be safe, don’t automatically invoke any wasm functions noInitialRun: false, locateFile(url: string): string { // Redirect the request for the wasm binary to whatever webpack gave us. if (url.endsWith('.wasm')) return wasmUrl; return url; }, onRuntimeInitialized() { m.then(resolve); } }); }); }

m.then(resolve)

Yeah, that seems less weird.

Also I bet we could extract the initialization into a helper method

Agreed. Good point.

developit · 2018-10-15T12:33:43Z

src/codecs/imagequant/processor.ts

+  return new Promise((resolve) => {
+    const m = imagequant({
+      // Just to be safe, don’t automatically invoke any wasm functions
+      noInitialRun: false,


Just noticed, this seems to do the opposite of what the comment above indicates.

no initial run = false --> initial run = true

Right, this should be true. Not sure how false got in here.

developit · 2018-10-15T12:34:25Z

src/codecs/processor-worker.ts

@@ -0,0 +1,41 @@
+import { expose } from 'comlink';


I like this a lot better than the per-encoder approach.

developit · 2018-10-15T12:38:56Z

src/codecs/processor.ts

+        if (needsWorker) self.clearTimeout(this._workerTimeout);
+
+        if (!this._worker && needsWorker) {
+          // Webpack's worker loader does magic here.


nit: worker-plugin

jakearchibald · 2018-10-15T14:18:13Z

@developit

m.then(resolve)

Unfortunately that doesn't work. The module resolves with itself, and resolve() will attempt to unwrap the thennables, which it can't because Emscripten is doing extra weird stuff.

surma · 2018-10-15T14:25:28Z

Yeah, see the comment and the linked GitHub issue as to why I did the weird delete thing.

surma · 2018-10-15T14:26:58Z

Seems like I never added the GitHub issue: emscripten-core/emscripten#6563

surma · 2018-10-16T21:05:40Z

codecs/mozjpeg_enc/example.html

-    const img = document.createElement('img');
-    img.src = blobURL;
-    document.body.appendChild(img);
+    for (let i = 0; i < 10; i++) {


Should we undo this change now that mozjpeg is fixed?

surma · 2018-10-17T12:37:04Z

src/codecs/decoders.ts

@@ -1,47 +1,21 @@
-import * as wasmWebp from './webp/decoder';
-import * as browserWebp from './webp/decoder';


Why did you inline these?

Feels like a drop in complexity while we only have one decoder. This file is smaller & IMO easier to follow as a result.

surma · 2018-10-17T12:39:15Z

src/codecs/processor-worker.ts

@@ -0,0 +1,41 @@
+import { expose } from 'comlink';


This is great for now. Just for documentation purposes: You said in person we’d probably wanna explore some form of code generation or something to make adding new codecs a bit easier in the future.

surma · 2018-10-17T12:40:03Z

src/codecs/processor.ts

+   * processing jobs require a worker (e.g. the main thread canvas encodes), use the needsWorker
+   * option to control this.
+   */
+  private static _processingJob(options: ProcessingJobOptions = {}) {


surma · 2018-10-17T12:44:48Z

src/codecs/processor.ts

+                this._worker.terminate();
+                this._worker = undefined;
+              },
+              10000,


Maybe make this a public instance prop?

surma · 2018-10-17T12:46:35Z

src/codecs/processor.ts

+  private _abortRejector?: (err: Error) => void;
+  private _busy = false;
+  private _latestJobId: number = 0;
+  private _workerTimeout: number = 0;


Can haz some comments on these fields? E.g. it wasn’t clear for the timeout is for. To kill a hanging worker or an unused one?

surma · 2018-10-17T12:49:14Z

src/codecs/processor.ts

+        // Wait for the operation to settle.
+        await returnVal.catch(() => {});
+
+        if (jobId === this._latestJobId) {


Rather make this an early return when the jobId doesn’t match?

surma · 2018-10-17T12:50:34Z

src/codecs/processor.ts

+        if (jobId === this._latestJobId) {
+          this._busy = false;
+
+          if (needsWorker) {


Super minor nit: I’d find this if more expressive if you did if (this._worker).

“If there is a worker, start the timeout countdown.”

surma · 2018-10-17T13:01:14Z

src/codecs/util.ts

+      onRuntimeInitialized() {
+        // An Emscripten is a then-able that resolves with itself, causing an infite loop when you
+        // wrap it in a real promise. Delete the `then` prop solves this for now.
+        // https://github.com/kripken/emscripten/issues/5820


… seconds

googlebot · 2018-10-20T11:56:11Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

jakearchibald · 2018-10-20T12:56:10Z

@surma feedback should be addressed now

surma

* Refactoring codecs * Plugging in new processor * Fixing decorator * MozJPEG free issue * Better worker aborting, and terminate workers that aren't used for 10 seconds * Better comment * Ooops, half-typed comment * Uncommenting problematic line * Surma fixed it! * Abstracting WASM initialisation * Better comment * Don't need this. * Adding ticket * noInitalRun * Reverting MozJPEG issue demo * Making a const for worker timeout * Inline docs * Bail early rather than nesting * Addressing nits

jakearchibald requested review from surma and developit October 13, 2018 15:55

jakearchibald changed the base branch from master to app-split October 13, 2018 15:57

jakearchibald commented Oct 13, 2018

View reviewed changes

jakearchibald mentioned this pull request Oct 14, 2018

Memory allocation fails after ~20 encodes #127

Closed

jakearchibald force-pushed the app-split branch from aab1fb8 to eda1658 Compare October 15, 2018 08:23

jakearchibald force-pushed the processor-worker branch 2 times, most recently from a4c173e to 95a1251 Compare October 15, 2018 10:27

developit reviewed Oct 15, 2018

View reviewed changes

surma requested changes Oct 17, 2018

View reviewed changes

jakearchibald added 13 commits October 20, 2018 12:55

Refactoring codecs

421fc3d

Plugging in new processor

88265a9

Fixing decorator

52c81b4

MozJPEG free issue

8b0c4b9

Better worker aborting, and terminate workers that aren't used for 10…

8d1f132

… seconds

Better comment

a14aa35

Ooops, half-typed comment

4df06a2

Uncommenting problematic line

aebd42b

Surma fixed it!

cf3ed36

Abstracting WASM initialisation

a25fb36

Better comment

12a4d12

Don't need this.

f6af028

Adding ticket

6e1e4aa

jakearchibald force-pushed the processor-worker branch from 7864739 to 6e1e4aa Compare October 20, 2018 11:56

jakearchibald added 6 commits October 20, 2018 13:00

noInitalRun

7ac14b1

Reverting MozJPEG issue demo

a712617

Making a const for worker timeout

a83ecca

Inline docs

c3aab4b

Bail early rather than nesting

a9b8cfc

Addressing nits

ca97fde

jakearchibald changed the base branch from app-split to master October 20, 2018 12:25

surma approved these changes Oct 20, 2018

View reviewed changes

jakearchibald merged commit 43def79 into master Oct 28, 2018

jakearchibald mentioned this pull request Oct 28, 2018

Worker termination #192

Closed

jakearchibald deleted the processor-worker branch October 30, 2018 13:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Two workers & worker termination #198

Two workers & worker termination #198

jakearchibald commented Oct 13, 2018 •

edited

Loading

jakearchibald commented Oct 13, 2018

jakearchibald Oct 13, 2018

jakearchibald Oct 13, 2018

developit Oct 15, 2018

surma Oct 17, 2018

jakearchibald Oct 13, 2018

surma commented Oct 13, 2018

jakearchibald commented Oct 13, 2018 via email

developit Oct 13, 2018

developit Oct 15, 2018

developit Oct 15, 2018

jakearchibald Oct 15, 2018

developit Oct 15, 2018

jakearchibald Oct 15, 2018

surma Oct 15, 2018

developit Oct 15, 2018

developit Oct 15, 2018

jakearchibald commented Oct 15, 2018

surma commented Oct 15, 2018

surma commented Oct 15, 2018

surma Oct 16, 2018

surma Oct 17, 2018

jakearchibald Oct 20, 2018

surma Oct 17, 2018

surma Oct 17, 2018

surma Oct 17, 2018

surma Oct 17, 2018

surma Oct 17, 2018

surma Oct 17, 2018

surma Oct 17, 2018

googlebot commented Oct 20, 2018

jakearchibald commented Oct 20, 2018

surma left a comment

		@@ -1,4 +1,4 @@
		import { EncodeOptions } from '../../src/codecs/mozjpeg/encoder';
		import { EncodeOptions } from '../../src/codecs/mozjpeg/encoder-meta';

		@@ -1,47 +1,21 @@
		import * as wasmWebp from './webp/decoder';
		import * as browserWebp from './webp/decoder';

Two workers & worker termination #198

Two workers & worker termination #198

Conversation

jakearchibald commented Oct 13, 2018 • edited Loading

jakearchibald commented Oct 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surma commented Oct 13, 2018

jakearchibald commented Oct 13, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakearchibald commented Oct 15, 2018

surma commented Oct 15, 2018

surma commented Oct 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Oct 20, 2018

jakearchibald commented Oct 20, 2018

surma left a comment

Choose a reason for hiding this comment

jakearchibald commented Oct 13, 2018 •

edited

Loading