Add tests for rendering to offscreen canvas #7029

davidswinegar · 2020-01-30T01:24:41Z

Add tests for using an offscreen canvas for Chart.js. These test rendering to an offscreen canvas and resizing the chart. Also fix various issues found during testing that blocked using an offscreen canvas.

src/helpers/helpers.dom.js

Add tests for using an offscreen canvas for Chart.js. These tests are almost identical to existing tests, but with offscreen canvas enabled.

benmccann · 2020-01-30T22:30:07Z

@davidswinegar is there anything else you need for web workers beyond this?

davidswinegar · 2020-01-30T22:54:03Z

@benmccann just added a final test that actually uses a real web worker, so this should be it! Thanks!

benmccann · 2020-01-30T23:45:46Z

Cool! Would you be able to add something to the documentation about it as well? The docs at docs/general/performance.md might be a good location (I'm assuming that the main usecase is speeding things up by drawing charts in parallel)

davidswinegar · 2020-01-31T01:37:10Z

@benmccann good suggestion, added that with an example.

package.json

benmccann

great docs! thanks so much for adding them!

benmccann · 2020-01-31T16:29:06Z

docs/general/performance.md

+const canvas = new HTMLCanvasElement();
+const offscreenCanvas = canvas.transferControlToOffscreen();
+
+const worker = new ChartWebWorker();


We should show what the definition of ChartWebWorker is

I don't think a example implementation of a web worker is in the scope of performance docs.
We could add a sample and link to that in here.
But I'd leave that for a later PR, so this one does not become a "never ending story".

I'll clear this up - the definition is just the code below it

benmccann · 2020-01-31T16:35:51Z

docs/general/performance.md

+
+By moving all Chart.js calculations onto a separate thread, the main thread can be freed up for other uses. Some tips and tricks when using Chart.js in a web worker:
+* Transferring data between threads can be expensive, so ensure that your config and data objects are as small as possible. Try generating them on the worker side if you can (workers can make HTTP requests!) or passing them to your worker as ArrayBuffers, which can be transferred quickly from one thread to another.
+* You can't transfer functions between threads, so if your config includes functions make sure to strip them out before passing them across worker boundaries.


Maybe it's still possible to use functions with web workers though if done properly? Perhaps we could update this line to something like:

You can't transfer functions between threads. This means if you can't pass a config with functions across worker boundaries. If you're passing the config from the main thread you'll need to strip out any functions. Alternatively, you can construct or fetch the config in the web worker.

Maybe it's still possible to use functions with web workers though if done properly?

Should be obvious.

To me the @davidswinegar's proposed text is clearer.

I'll clarify this!

kurkle · 2020-02-03T06:08:37Z

docs/general/performance.md


+## Render Chart.js in a web worker (Chrome only)
+
+Chrome recently added the ability to [transfer rendering control of a canvas](https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement/transferControlToOffscreen) to a web worker. Web workers can use the [OffscreenCanvas API](https://developer.mozilla.org/en-US/docs/Web/API/OffscreenCanvas) to render from a web worker onto canvases in the DOM. Chart.js is a canvas-based library and supports rendering in a web worker - just pass an OffscreenCanvas into the Chart constructor instead of a Canvas element. Note that as of today, this API is only supported in Chrome.


recently might not hold for long. Maybe state the version instead (69)?

kurkle · 2020-02-03T06:14:01Z

docs/general/performance.md

+const canvas = new HTMLCanvasElement();
+const offscreenCanvas = canvas.transferControlToOffscreen();
+
+const worker = new ChartWebWorker();


I don't think a example implementation of a web worker is in the scope of performance docs.
We could add a sample and link to that in here.
But I'd leave that for a later PR, so this one does not become a "never ending story".

davidswinegar · 2020-02-04T01:56:59Z

This seems to keep failing in Windows. I'm going to see if I can get my hands on a Windows machine to debug it, but if you guys have any insights as to what it might be please let me know.

benmccann · 2020-02-04T03:59:10Z

karma.conf.js

 const istanbul = require('rollup-plugin-istanbul');
 const resolve = require('rollup-plugin-node-resolve');
 const builds = require('./rollup.config');
+const babel = require('rollup-plugin-babel');


this looks new and unused?

benmccann

Oh, actually I just noticed it looks like this might have done something to the code coverage. It says coverage decreased 90% to 3%

davidswinegar · 2020-02-04T19:28:54Z

^ I think it's possible that the coverage fell because of the failing windows build, so hopefully fixing that will fix the coverage.

kurkle · 2020-02-04T19:29:10Z

karma.conf.js

-				commonjs()
+				commonjs(),
+				// The two plugins below are required to load the BasicChartWebWorker.
+				stylesheet({extract: true}),


The windows failure and coverage drop is probably related to this / these changes. Its running only one test.

TOTAL: 1 SUCCESS
TOTAL: 1 SUCCESS

stylesheet is no longer necessary after rebasing against master

I was wondering if webWorkerLoader and resolve/commonjs are interfering with each other, but I see other projects using all three online. Also, I added #7142 to see if keeping commonjs from running on the web worker import would help, but it didn't seem to have any effect

benmccann · 2020-02-14T04:16:42Z

@davidswinegar this PR will have to be rebased. sorry for the trouble

benmccann · 2020-02-21T04:45:32Z

test/specs/platform.basic.tests.js

@@ -0,0 +1,93 @@
+import BasicChartWebWorker from 'web-worker:../BasicChartWebWorker';


Is web-worker: a special syntax? I can't find anything about it online. I tried to check out this PR locally. It was pretty easy to rebase, but gulp lint is choking on this line. Not sure if I lost something while trying to rebase

Ah, I figured out where this is coming from: https://www.npmjs.com/package/rollup-plugin-web-worker-loader

You can stop eslint from choking on this line by changing it to:

import BasicChartWebWorker from 'web-worker:../BasicChartWebWorker'; // eslint-disable-line import/no-unresolved

benmccann · 2020-02-22T05:22:32Z

I put together a rebased version for you: https://github.com/benmccann/Chart.js/tree/PR7029

The tests still fail though

wrong pr

benmccann · 2020-02-22T22:19:20Z

@davidswinegar we merged the non-test changes for you here: #7146

Hopefully we can figure out why these tests are failing and merge the rest of the PR with your help though

davidswinegar · 2020-02-24T21:42:43Z

@benmccann Great, thank you! I've stepped away from this for a while and used my own forked version of Chart.js but will likely be returning to it in the next week or two to see if I can get those tests working - it's taking me a while to get a Windows VM up and running to debug.

benmccann · 2020-02-25T04:28:57Z

It's weird the tests fail only on the Windows CI. They fail for me locally on my Linux machine, but maybe that's because I rebased? I wonder if they'll fail for you on the rebased version https://github.com/benmccann/Chart.js/commits/PR7029

darionco · 2020-03-02T17:56:41Z

Posted a solution here:
darionco/rollup-plugin-web-worker-loader#9

Cheers!

benmccann · 2020-03-02T18:19:03Z

@darionco thanks so much for the pointer! That got it building with the tests passing. Unfortunately it's still only running one test. But that's good progress at least!

https://github.com/benmccann/Chart.js/tree/PR7029

darionco · 2020-03-02T18:31:22Z

to fix that simply change line 13 in your platform.basic.tests.js file from:

fit('supports choosing the BasicPlatform in a web worker', function(done) {

to:

it('supports choosing the BasicPlatform in a web worker', function(done) {

as per jasmine's documentation:
https://davidtang.io/2016/01/03/controlling-which-tests-run-in-jasmine.html

when I do that I get:

Firefox 73.0.0 (Mac OS X 10.15.0) ERROR: 'Failed to create chart: can't acquire context from the given item'
Chrome 80.0.3987 (Mac OS X 10.15.3): Executed 1028 of 1028 SUCCESS (19.878 secs / 15.415 secs)
Chrome 80.0.3987 (Mac OS X 10.15.3): Executed 1028 of 1028 SUCCESS (19.878 secs / 15.415 secs)
Firefox 73.0.0 (Mac OS X 10.15.0): Executed 1023 of 1028 (skipped 5) SUCCESS (22.318 secs / 20.155 secs)
TOTAL: 2051 SUCCESS
TOTAL: 2051 SUCCESS

cheers!

benmccann · 2020-03-02T20:29:49Z

@darionco you're a lifesaver! thank you so much!!

kurkle reviewed Jan 30, 2020

View reviewed changes

src/helpers/helpers.dom.js Outdated Show resolved Hide resolved

benmccann changed the title ~~Add tests for rendering to offscreen canvas~~ Add support for rendering to offscreen canvas Jan 30, 2020

benmccann added the type: enhancement label Jan 30, 2020

Add tests for rendering to offscreen canvas

9b95fb4

Add tests for using an offscreen canvas for Chart.js. These tests are almost identical to existing tests, but with offscreen canvas enabled.

davidswinegar force-pushed the dwinegar-offscreen-canvas-tests branch from 6954e81 to 9b95fb4 Compare January 30, 2020 21:35

Really add chart in a web worker example

334acc8

davidswinegar force-pushed the dwinegar-offscreen-canvas-tests branch from e1831a6 to 334acc8 Compare January 30, 2020 23:02

Add docs on rendering in a web worker

29823fc

davidswinegar force-pushed the dwinegar-offscreen-canvas-tests branch from 1bebae4 to 29823fc Compare January 31, 2020 01:40

etimberg reviewed Jan 31, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

benmccann reviewed Jan 31, 2020

View reviewed changes

benmccann mentioned this pull request Feb 1, 2020

Fix some JSDoc errors #7026

Merged

kurkle reviewed Feb 3, 2020

View reviewed changes

benmccann reviewed Feb 4, 2020

View reviewed changes

Address comments

b8f29a8

davidswinegar force-pushed the dwinegar-offscreen-canvas-tests branch from ca21377 to b8f29a8 Compare February 4, 2020 17:46

benmccann approved these changes Feb 4, 2020

View reviewed changes

benmccann suggested changes Feb 4, 2020

View reviewed changes

kurkle reviewed Feb 4, 2020

View reviewed changes

benmccann reviewed Feb 21, 2020

View reviewed changes

benmccann mentioned this pull request Feb 22, 2020

Error: Unexpected token darionco/rollup-plugin-web-worker-loader#9

Closed

benmccann mentioned this pull request Feb 22, 2020

WebWorker support #7146

Merged

kurkle previously approved these changes Feb 22, 2020

View reviewed changes

benmccann changed the title ~~Add support for rendering to offscreen canvas~~ Add tests for rendering to offscreen canvas Mar 1, 2020

benmccann added type: chore and removed type: enhancement labels Mar 1, 2020

benmccann mentioned this pull request Mar 2, 2020

Add tests for rendering to offscreen canvas #7175

Merged

etimberg closed this in #7175 Mar 6, 2020


		## Render Chart.js in a web worker (Chrome only)

		Chrome recently added the ability to [transfer rendering control of a canvas](https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement/transferControlToOffscreen) to a web worker. Web workers can use the [OffscreenCanvas API](https://developer.mozilla.org/en-US/docs/Web/API/OffscreenCanvas) to render from a web worker onto canvases in the DOM. Chart.js is a canvas-based library and supports rendering in a web worker - just pass an OffscreenCanvas into the Chart constructor instead of a Canvas element. Note that as of today, this API is only supported in Chrome.

		@@ -0,0 +1,93 @@
		import BasicChartWebWorker from 'web-worker:../BasicChartWebWorker';

Add tests for rendering to offscreen canvas #7029

Add tests for rendering to offscreen canvas #7029

Uh oh!

Conversation

davidswinegar commented Jan 30, 2020

Uh oh!

Uh oh!

benmccann commented Jan 30, 2020

Uh oh!

davidswinegar commented Jan 30, 2020

Uh oh!

benmccann commented Jan 30, 2020

Uh oh!

davidswinegar commented Jan 31, 2020

Uh oh!

Uh oh!

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

davidswinegar commented Feb 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benmccann left a comment

Choose a reason for hiding this comment

Uh oh!

davidswinegar commented Feb 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benmccann Feb 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benmccann commented Feb 14, 2020

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!

benmccann commented Feb 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benmccann commented Feb 22, 2020

Uh oh!

davidswinegar commented Feb 24, 2020

Uh oh!

benmccann commented Feb 25, 2020

Uh oh!

darionco commented Mar 2, 2020

Uh oh!

benmccann commented Mar 2, 2020

Uh oh!

darionco commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benmccann commented Mar 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

benmccann Feb 22, 2020 •

edited

Loading

benmccann commented Feb 22, 2020 •

edited

Loading

darionco commented Mar 2, 2020 •

edited

Loading