1126457 - Implement a worker utility for DevTools to make adding new workers easier

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect, P3)

Description

[Victor Porof [:vporof][:vp]]

Currently, we do *a lot* of data massaging after receiving it when a recording is finished. This is a big source of jank, especially for longer recordings.

All the methods in that module can be moved into a worker. However, it's not clear yet if the cloning overhead will negate any other potential benefits.

Comment 1

[Victor Porof [:vporof][:vp]]

Not necessary for v2.

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1126457-worker-helper.patch

Worker utility to easily throw functions into a worker. If this is a perf improvement, ill throw this up for review.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2860cb0ba307

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

All of RecordingUtils functions are several orders of magnitude slower when put in a worker. Most of them only cost 1-2ms, with some profiler filtering being around 20ms. Using the worker, a big profile is 3 seconds or so for the same result. Transfer cost hurts.

Measuring times for idle while loading google.com and one for octane benchmarks:

Mostly idle, google.com
Fetch Profile (request to response from front): 602ms
Fetch Profile (server to nsIProfiler): 248ms
Rendering CallTree: 1079ms

Full octane
Fetch Profile (request to response from front): 19634ms
Fetch Profile (server to nsIProfiler): 9153
Rendering callTree: 9597


We still have a third of our time getting profiles from the profiler, and another third getting that to the front, with the last third being rendering the js call tree. I wonder if the call tree could benefit from any worker stuff.

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8601226 [details] [diff] [review]
1126457-worker-helper.patch

So some tests comparing current worker in the graphs.jsm to the worker utility attached -- was afraid it'd be a bit slower, but seems slightly faster even.

n=10^6
[#0] current-worker Cycles:25 Average:558.02 Median:549.60 stddev:38.05 (6.9%) stddev-sans-first:35.29
Values: 634.6 582.1 586.1 573.6 522.3 661.3 571.6 590.9 506.8 566.5 543.3 538.4 547.7 557.0 564.9 605.2 568.4 521.8 519.8 537.4 502.8 549.6 529.1 530.3 538.9
 
[#1] devtools-worker Cycles:25 Average:521.11 Median:509.38 stddev:30.67 (6.0%) stddev-sans-first:31.14
Values: 537.1 555.8 536.7 551.7 499.6 509.4 514.9 512.7 501.3 500.1 535.1 556.7 499.4 498.9 623.2 505.4 499.3 511.8 492.3 500.4 497.5 502.3 568.5 507.0 510.5

n=10^4
[#0] current-worker Cycles:25 Average:34.11 Median:29.33 stddev:11.57 (39.5%) stddev-sans-first:11.69
Values: 25.9 27.9 52.4 29.3 22.2 28.7 61.8 29.4 31.1 51.0 34.4 43.0 26.2 28.5 31.7 25.1 29.3 26.8 49.5 48.7 29.3 15.1 31.9 49.2 24.4

[#1] devtools-worker Cycles:25 Average:19.47 Median:18.56 stddev:4.90 (26.4%) stddev-sans-first:5.00
Values: 20.3 18.6 16.7 15.1 20.7 22.3 17.2 22.0 14.6 16.9 18.9 29.4 30.8 19.3 32.5 17.1 14.1 18.7 15.0 14.0 20.9 18.4 16.5 17.3 19.3

n=10^3
[#0] current-worker Cycles:25 Average:24.06 Median:20.06 stddev:11.73 (58.5%) stddev-sans-first:11.94
Values: 28.7 26.9 26.2 10.4 43.4 11.7 21.8 22.9 18.8 30.1 20.1 25.8 37.9 47.8 34.9 13.4 14.5 18.0 12.5 18.4 53.5 14.6 19.7 18.9 10.7

[#1] devtools-worker Cycles:25 Average:18.23 Median:17.76 stddev:4.69 (26.4%) stddev-sans-first:4.78
Values: 16.2 22.9 12.5 20.8 29.1 22.0 24.7 14.3 14.8 13.1 14.4 11.0 22.4 16.1 25.0 19.5 17.8 15.8 17.8 20.9 15.3 19.0 13.2 13.6 23.6

n=10^3, looped 100 times per test (to take advantage of caching or anything like that, more realistic to how the graph worker is called anyway)
[#0] current-worker Cycles:25 Average:1113.77 Median:1118.67 stddev:102.63 (9.2%) stddev-sans-first:91.00
Values: 869.2 1063.4 991.5 965.3 1100.8 1120.4 1103.8 1088.0 1141.2 1075.2 1160.2 1054.2 1105.3 1145.1 1208.4 1244.5 1107.5 1450.7 1133.4 1118.7 1099.6 1112.9 1115.0 1169.2 1100.7

[#1] devtools-worker Cycles:25 Average:1080.41 Median:1082.30 stddev:50.54 (4.7%) stddev-sans-first:51.54
Values: 1094.3 1007.8 998.7 1045.0 1062.6 1013.8 1032.3 1021.6 1068.6 1086.4 1078.5 1082.4 1061.1 1069.7 1219.5 1112.3 1182.6 1100.5 1108.2 1075.1 1082.3 1128.0 1064.3 1088.5 1126.3

// And again, 10^3, loops 100 times, but this does not use a try/catch in the worker-helper -- doesn't seem to matter
[#1] devtools-worker  Cycles:25  Average:1074.32  Median:1138.67  stddev:103.71 (9.1%)  stddev-sans-first:100.79
Values: 1227.6  956.1  962.6  953.3  971.8  985.8  1021.1  991.1  985.0  990.5  1027.4  1034.1  1035.0  1035.8  1193.7  1097.4  1115.7  1401.5  1111.4  1111.1  1156.2  1102.1  1116.7  1138.7  1136.5
-

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8601226 [details] [diff] [review]
1126457-worker-helper.patch

Review of attachment 8601226 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like a good abstraction in general, just a few notes.  Also +1 for having numbers to compare speed - can you attach the test case you used to generate that data?

::: browser/devtools/shared/test/browser_devtools-worker-02.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// Tests that the devtools/shared/worker communicates properly
> +// when loaded as a JSM.

Looking at the differences between these two files, I see this as a single test with two tasks inside of devtools-worker-01.js in which DevToolsWorker is scoped within each function as loaded via require or Cu.import

::: browser/devtools/shared/test/browser_devtools-worker-03.js
@@ +15,5 @@
> +  }
> +
> +  let worker = new DevToolsWorker(WORKER_URL);
> +  try {
> +    let results = yield worker.performTask("plotTimestampsGraph", {});

Could use a comment explaining why this input causes it to throw

::: browser/devtools/shared/widgets/Graphs.jsm
@@ +2123,5 @@
>     * Performs the given task in a chrome worker, assuming it exists.
>     *
>     * @param string task
>     *        The task name. Currently supported: "plotTimestampsGraph".
> +   * @param data args

This comment got flipped, should be `@param any data`

::: browser/devtools/shared/widgets/GraphsWorker.js
@@ +19,5 @@
> +createTask(self, "plotTimestampsGraph", function ({ timestamps, interval, duration }, callback) {
> +  let plottedData = plotTimestamps(timestamps, interval);
> +  let plottedMinMaxSum = getMinMaxAvg(plottedData, timestamps, duration);
> +
> +  callback(null, { plottedData, plottedMinMaxSum });

This node-style callback seems foreign here.  Is this used elsewhere in devtools for error handling?  Maybe these workers should just throw inside of the function if there is an error.  With the current implementation there is no distinction between `throw new Error("noobar");` and `callback(new Error("foobar"))`, and I think the former is more guessable.

::: browser/devtools/shared/worker.js
@@ +44,5 @@
> + * @param {any} data
> + *        Data to be passed into the task implemented by the worker.
> + * @return {Promise}
> + */
> +DevToolsWorker.prototype.performTask = function DevToolsWorkerPerformTask (task, data) {

the `transferrable` parameter from the implementation in Graphs.jsm isn't being accepted here.  Looks like it should be an extra parameter to postMessage

@@ +49,5 @@
> +  if (this._destroyed) {
> +    return Promise.reject("Cannot call performTask on a destroyed DevToolsWorker");
> +  }
> +  let worker = this._worker;
> +  let id = generateUUID().toString();

In the old implementation in graphs.jsm `id` was kept as incremented counter (_graphUtilsTaskId).  Is there a reason for the change?

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: worker test patch

Ok -- so before, I didn't run metrics on the difference in using this worker on RecordingUtils itself completely. So got scientific this time. Below are results for each of the RecordingUtils functions, first how it is currently, and second with the DevToolsWorker. The utils dealing with timeline data process 40 items, looped 20 times, as this occurs frequently every 200ms or so, while the profiler utils deal with a giant amount of samples at the very end -- measured for both small profiles (5000 samples) and large (43000, or 43s of recording, and this is with Octane).

Compared results with current implementation vs worker implementation:
offsetSampleTimes (small): 24 vs 19
offsetSampleTimes (large): 171 vs 324
offsetMarkerTimes: 8 vs 33
offsetAndScaleTimestamps: 1.6 vs 5.36
filterSamples (small): 23 vs 16
filterSamples (large): 225 vs 244

For the smaller, fast events (adjusting marker times every ~200ms), definitely not worth putting in a worker. Even for the larger functions, the non-worker wins out (the worker versions have a very large variations). offsetSampleTimes will be for compatibility only once we figure out time syncing, and filterSamples is only used in old geckos as well. Is it worth allowing these functions to be called as a worker for the big profile functions, just to prevent blocking? Does transferring the data over to a worker incur blocking? Also the fake profiles used here are much smaller than real ones, so the overhead would probably be greater as well.

Attached is the patch needed, on top of the DevToolsWorker implementation, to expose everything needed to run this test: https://gist.github.com/jsantell/7833795ad07d94476b9f

And that test is run inside the talos framework thing I run locally: https://github.com/jsantell/metaperf

If there was a good way to just wrap a function and create Workers without needing a URL, we could probably quickly A/B test many functions to see if workers improve speed.

******

[#0] current-worker-offsetSampleTimes-small  Cycles:25  Average:24.32  Median:23.20  stddev:4.46 (19.2%)  stddev-sans-first:4.02
Values: 34.4  28.5  29.8  23.1  20.3  25.0  22.5  24.5  23.5  23.2  29.2  18.5  18.9  17.9  24.9  34.3  22.7  19.1  23.0  21.3  25.7  25.7  22.1  28.6  21.1

[#1] current-worker-offsetSampleTimes-large  Cycles:25  Average:171.22  Median:170.00  stddev:8.95 (5.3%)  stddev-sans-first:8.95
Values: 180.0  162.7  192.6  181.1  172.2  158.4  165.4  169.2  163.8  165.2  173.3  167.5  182.0  158.9  176.1  190.4  164.5  172.3  163.8  170.5  172.3  169.1  161.4  177.8  170.0

[#2] current-worker-offsetMarkerTimes  Cycles:25  Average:8.02  Median:5.29  stddev:12.42 (234.7%)  stddev-sans-first:12.68
Values: 6.3  5.1  5.6  5.2  5.1  67.5  5.3  5.8  5.0  5.1  5.3  5.3  8.5  5.2  5.1  6.1  5.2  5.3  5.4  5.4  5.4  5.5  5.1  5.1  6.7

[#3] current-worker-offsetAndScaleTimestamps  Cycles:25  Average:1.60  Median:1.57  stddev:0.14 (8.7%)  stddev-sans-first:0.14
Values: 1.6  1.6  1.6  1.5  1.7  1.4  1.6  1.8  1.5  1.6  1.7  1.5  2.1  1.5  1.5  1.7  1.5  1.6  1.5  1.5  1.5  1.6  1.5  1.6  1.8

[#4] current-worker-filterSamples-small  Cycles:25  Average:23.43  Median:24.95  stddev:4.37 (17.5%)  stddev-sans-first:4.42
Values: 26.4  17.0  22.8  24.2  25.3  16.2  25.9  20.1  23.1  25.7  27.0  24.9  23.8  16.3  23.3  32.5  25.7  21.1  15.8  26.2  27.5  26.3  15.7  25.9  27.1

[#5] current-worker-filterSamples-large  Cycles:25  Average:225.27  Median:221.88  stddev:14.39 (6.5%)  stddev-sans-first:14.52
Values: 236.2  212.5  234.3  214.3  221.1  254.2  221.8  234.5  227.5  214.7  230.3  218.9  273.2  215.5  227.0  233.3  221.9  205.9  222.6  228.7  219.7  216.5  213.8  209.7  223.6

[#6] dt-worker-offsetSampleTimes-small  Cycles:25  Average:19.29  Median:18.84  stddev:1.85 (9.8%)  stddev-sans-first:1.80
Values: 21.9  18.9  19.8  17.5  18.3  25.3  17.8  18.4  18.2  19.4  18.8  18.0  22.4  19.1  18.9  21.2  17.8  18.1  21.4  19.8  18.4  17.8  18.1  17.9  19.4

[#7] dt-worker-offsetSampleTimes-large  Cycles:25  Average:324.11  Median:294.19  stddev:133.92 (45.5%)  stddev-sans-first:136.48
Values: 279.9  302.1  278.4  291.5  297.1  306.7  301.3  298.9  291.3  296.3  292.4  293.0  264.2  351.0  295.2  279.8  961.3  293.9  311.4  291.0  293.5  292.2  304.7  294.2  341.4

[#8] dt-worker-offsetMarkerTimes  Cycles:25  Average:33.58  Median:9.42  stddev:117.58 (1248.2%)  stddev-sans-first:120.01
Values: 10.5  10.6  10.1  9.8  14.2  10.8  9.4  9.6  9.7  10.0  9.9  9.4  9.7  9.6  9.6  9.4  10.0  10.3  9.4  10.0  9.6  9.7  9.6  597.9  10.5

[#9] dt-worker-offsetAndScaleTimestamps  Cycles:25  Average:5.36  Median:5.26  stddev:0.48 (9.2%)  stddev-sans-first:0.49
Values: 5.2  5.3  5.2  5.2  5.3  5.5  5.2  5.0  5.5  5.3  5.5  5.0  5.2  5.1  5.0  5.1  5.3  5.6  5.3  5.0  5.3  5.2  5.5  7.3  6.2

[#10] dt-worker-filterSamples-small  Cycles:25  Average:16.42  Median:16.15  stddev:1.30 (8.1%)  stddev-sans-first:1.33
Values: 17.0  16.7  16.3  15.4  20.1  19.4  16.2  15.5  15.3  15.6  15.8  15.4  16.7  17.0  15.0  16.3  16.4  15.7  16.0  15.7  15.5  16.9  15.4  19.0  16.4

[#11] dt-worker-filterSamples-large  Cycles:25  Average:244.99  Median:207.80  stddev:150.18 (72.3%)  stddev-sans-first:153.33
Values: 220.8  210.8  207.4  228.4  209.7  371.9  208.5  229.1  947.5  204.1  207.8  208.0  198.6  203.9  204.3  208.3  195.3  207.4  217.7  206.8  200.3  197.2  204.6  204.9  221.7

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8601226 [details] [diff] [review]
1126457-worker-helper.patch

Review of attachment 8601226 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/shared/test/browser_devtools-worker-02.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// Tests that the devtools/shared/worker communicates properly
> +// when loaded as a JSM.

Yeah sounds good, could just all be in one file

::: browser/devtools/shared/widgets/GraphsWorker.js
@@ +19,5 @@
> +createTask(self, "plotTimestampsGraph", function ({ timestamps, interval, duration }, callback) {
> +  let plottedData = plotTimestamps(timestamps, interval);
> +  let plottedMinMaxSum = getMinMaxAvg(plottedData, timestamps, duration);
> +
> +  callback(null, { plottedData, plottedMinMaxSum });

Errors are caught and propagated as a rejected promise, but if handling anything async in a worker, that wouldn't work. You're right, we don't do node-style callbacks really. Can change it to a callback that just takes one value, and if that value has an `error` property, it's a reject? (look at worker-helper.js here)

::: browser/devtools/shared/worker.js
@@ +44,5 @@
> + * @param {any} data
> + *        Data to be passed into the task implemented by the worker.
> + * @return {Promise}
> + */
> +DevToolsWorker.prototype.performTask = function DevToolsWorkerPerformTask (task, data) {

That wasn't used, and there is no documentation on MDN other than specifying it in postMessage (where does it even go?). If this is a need for anyone using a DevToolsWorker (maybe there's a better name), we can add it then

@@ +49,5 @@
> +  if (this._destroyed) {
> +    return Promise.reject("Cannot call performTask on a destroyed DevToolsWorker");
> +  }
> +  let worker = this._worker;
> +  let id = generateUUID().toString();

Ah, originally this was a global thing. Can remove the overhead of creating UUIDs with just a simple incrementor

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #7)
> ::: browser/devtools/shared/widgets/GraphsWorker.js
> @@ +19,5 @@
> > +createTask(self, "plotTimestampsGraph", function ({ timestamps, interval, duration }, callback) {
> > +  let plottedData = plotTimestamps(timestamps, interval);
> > +  let plottedMinMaxSum = getMinMaxAvg(plottedData, timestamps, duration);
> > +
> > +  callback(null, { plottedData, plottedMinMaxSum });
> 
> Errors are caught and propagated as a rejected promise, but if handling
> anything async in a worker, that wouldn't work. You're right, we don't do
> node-style callbacks really. Can change it to a callback that just takes one
> value, and if that value has an `error` property, it's a reject? (look at
> worker-helper.js here)

Hm yeah I based the idea that we should just throw off of the implementation in worker-helper but that won't work in async cases as you point out.  What about if you just pass in an Error object as the argument to callback and then in worker-helper check for `response instanceof Error`?

> ::: browser/devtools/shared/worker.js
> @@ +44,5 @@
> > + * @param {any} data
> > + *        Data to be passed into the task implemented by the worker.
> > + * @return {Promise}
> > + */
> > +DevToolsWorker.prototype.performTask = function DevToolsWorkerPerformTask (task, data) {
> 
> That wasn't used, and there is no documentation on MDN other than specifying
> it in postMessage (where does it even go?). If this is a need for anyone
> using a DevToolsWorker (maybe there's a better name), we can add it then

Sounds fine, please remove the argument from the Graphs.jsm function

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> (In reply to Jordan Santell [:jsantell] [@jsantell] from comment #7)
> > Errors are caught and propagated as a rejected promise, but if handling
> > anything async in a worker, that wouldn't work. You're right, we don't do
> > node-style callbacks really. Can change it to a callback that just takes one
> > value, and if that value has an `error` property, it's a reject? (look at
> > worker-helper.js here)
> 
> Hm yeah I based the idea that we should just throw off of the implementation
> in worker-helper but that won't work in async cases as you point out.  What
> about if you just pass in an Error object as the argument to callback and
> then in worker-helper check for `response instanceof Error`?

Alternatively, we could have the createTask functions return a promise that could be rejected in the case of an error.

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #6)
> If there was a good way to just wrap a function and create Workers without
> needing a URL, we could probably quickly A/B test many functions to see if
> workers improve speed.
> 

I've done something similar before with Blobs and createObjectURL - something like this: http://www.briangrinstead.com/blog/load-web-workers-without-a-javascript-file

Comment 11

[Jordan Santell [:jsantell] [@jsantell]]

Whoa, I'll see if I can add support for anonymous Workers like that. Would make wrapping current functions easy and easy to run tests.

I'll update the patch in a bit with the changes, I don't think having the worker functions return a promise will be that useful, and since it's in a worker, one doesn't really have to make things async for performance reasons (but may use something that returns a promise from another module), so can check those states too.

Comment 12

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1126457-worker-helper.patch

All comments addressed -- worker tasks can now return a primitive, or a promise, or an error, or define a second argument that's a function, that takes a primitive, or a promise, or an error, etc.

and also to test, implemented `workerify` -- takes any pure function, that returns a primitive or promise, and yields the response. This is pretty cool. This means we can just do any function in talos test like:

function *ABTest (fn) {
  let workerFn = workerify(fn);
  timer.start("original");
  fn();
  timer.stop("original");
  timer.start("worker");
  yield workerFn();
  timer.stop("worker");
}

Comment 13

[Jordan Santell [:jsantell] [@jsantell]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ce1d4d6bc73

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8601799 [details] [diff] [review]
1126457-worker-helper.patch

Review of attachment 8601799 [details] [diff] [review]:
-----------------------------------------------------------------

Cool idea, the createTask / performStuff will be a nice utility for quickly spinning up workers on the frontend, and workerify should be helpful for quickly comparing performance.  Just a couple more notes about the API I'd like to go through before landing

::: browser/devtools/shared/worker-helper.js
@@ +7,5 @@
> + * This file is to only be included by ChromeWorkers. This exposes
> + * a `createTask` function to workers to register tasks for communication
> + * back to `devtools/shared/worker`.
> + *
> + * Tasks can be send their responses in several ways.

This takes being liberal in what types of input we accept to a whole new level :).  It doesn't add much complexity to the implementation, although as discussed it's hard to imagine someone using both a callback and a promise.

My gut feeling, which seems supported by the header comment in workerify about callback-versions being weird, is that we should remove the callback functionality completely and require a return value.  If you need async processing, then that return value can be a promise.  Many worker tasks will probably be sync anyway, since it's in a worker.  This would also get rid of the quirk where including a second argument in the function changes the behavior.  I'm happy to discuss this further if you think including the callback adds value.

::: browser/devtools/shared/worker.js
@@ +16,5 @@
> +      this.EXPORTED_SYMBOLS = ["DevToolsWorker"];
> +  }
> +}).call(this, function (require, exports, module, { Ci, Cc }, ChromeWorker ) {
> +
> +let INC = 0;

Maybe name this MESSAGE_ID / MESSAGE_COUNTER or similar

@@ +80,5 @@
> +/**
> + * Takes a function and returns a Worker-wrapped version of the same function.
> + * Returns a promise upon resolution.
> + *
> + * * * * ! ! ! This should only be used for tests or A/B testing performance ! ! ! * * * * * *

Maybe we could include something like `console.log("Creating a function with workerify, don't do this in production");` inside the implementation, in case someone doesn't read this warning.

@@ +83,5 @@
> + *
> + * * * * ! ! ! This should only be used for tests or A/B testing performance ! ! ! * * * * * *
> + *
> + * The original function must:
> + * 

NIt: whitespace

@@ +105,5 @@
> +  let url = URL.createObjectURL(blob);
> +  let worker = new DevToolsWorker(url);
> +
> +  let wrapperFn = function (data, callback) {
> +    if (callback) {

Have you needed the callback?  Seems like it'd be just as easy to do:

var workerVersion = workerify(myFn);
workerVersion(data).then(result => {});

as

var workerVersion = workerify(myFn);
workerVersion(data, result => { });

Doesn't really hurt anything to leave it here (especially since it's test-only), but just curious if there's a need for it

Comment 15

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8601799 [details] [diff] [review]
1126457-worker-helper.patch

Review of attachment 8601799 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/shared/worker-helper.js
@@ +7,5 @@
> + * This file is to only be included by ChromeWorkers. This exposes
> + * a `createTask` function to workers to register tasks for communication
> + * back to `devtools/shared/worker`.
> + *
> + * Tasks can be send their responses in several ways.

As a hilarious experiment in continuation styles, this was fun. But I agree -- the callback situation is strange. Do we call the callback anyway with the value, in the event of a workerified function? And passing an error/promise/primitive in the callback etc.

I'm surprised all that works, to be honest. But yes, let's just do return value and promises. This may require reworking some functions to be "workerified", but that's probably going to be the case anyway, as it requires one argument. This reduces a lot of complexity in others potentially using this.

::: browser/devtools/shared/worker.js
@@ +80,5 @@
> +/**
> + * Takes a function and returns a Worker-wrapped version of the same function.
> + * Returns a promise upon resolution.
> + *
> + * * * * ! ! ! This should only be used for tests or A/B testing performance ! ! ! * * * * * *

Ah yes I was going to add a console.warn here

@@ +105,5 @@
> +  let url = URL.createObjectURL(blob);
> +  let worker = new DevToolsWorker(url);
> +
> +  let wrapperFn = function (data, callback) {
> +    if (callback) {

Removing callbacks anyway

Comment 16

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1126457-worker-helper.patch

comments addressed, callback-style worker removed

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8602237 [details] [diff] [review]
1126457-worker-helper.patch

Review of attachment 8602237 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/shared/worker-helper.js
@@ +71,5 @@
> +      return;
> +    }
> +
> +    try {
> +      let results;

looks like this line can be removed

::: browser/devtools/shared/worker.js
@@ +96,5 @@
> + * @param {function} fn
> + * @return {function}
> + */
> +function workerify (fn) {
> +  console.warn(`\`workerify\` should only be used in tests or measuring performance.

Will this throw in the case of a jsm?  I think you might need to import console in that case similar to how Promise is included

Comment 18

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8602237 [details] [diff] [review]
1126457-worker-helper.patch

Review of attachment 8602237 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/shared/worker-helper.js
@@ +71,5 @@
> +      return;
> +    }
> +
> +    try {
> +      let results;

This should catch an error thrown synchronously -- like passing in undefined on something that takes an array of numbers to average. Talking to Shu, it sounds like this does not prevent the worker from going into the JIT. If this isn't handled here, the only other option is the general onerror handler for the worker, which cannot differentiate between which task caused it

::: browser/devtools/shared/worker.js
@@ +96,5 @@
> + * @param {function} fn
> + * @return {function}
> + */
> +function workerify (fn) {
> +  console.warn(`\`workerify\` should only be used in tests or measuring performance.

Ah you're right -- Added a little test in the JSM test to test that workerify works there too.

Comment 19

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1126457-worker-helper.patch

Comment 20

[Jordan Santell [:jsantell] [@jsantell]]

Worker helper landed in fx-team; it seems that RecordingUtils does not get a perf benefit from offloading into a worker, but it may help in preventing locking responsiveness -- investigation can continue in bug 1161693 for that.

https://hg.mozilla.org/integration/fx-team/rev/c2da2db00b08

Comment 21

[Brian Grinstead [:bgrins]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #20)
> Worker helper landed in fx-team; it seems that RecordingUtils does not get a
> perf benefit from offloading into a worker, but it may help in preventing
> locking responsiveness -- investigation can continue in bug 1161693 for that.

OK, renaming the bug

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c2da2db00b08