Consider removing additional arguments to postTask

[esprehn]

It's not clear why anyone would want to pass additional arguments to the callback through postTask itself instead of using an arrow function. I think setTimeout did this because it predated arrow functions? I think it would be nice to avoid having that extra complexity in postTask and to use arrow functions instead.

postTask(callback, undefined, 1, 2)

becomes

postTask(() => callback(1, 2))

This is also safer for long term evolution of the API. If postTask was to change what arguments it passes into the callback it would break the first form because it would shift down the extra arguments. The second form let's us change what postTask forwards in the future in a backwards compatible way.

@shaseley

[shaseley]

Coincidentally, I was just discussing this with @developit this week. I agree and think that would simplify things and be more future-proof w.r.t. passing things to the callback (e.g. signal perhaps).

One thing Jason brought up was if in the (perhaps distant) future we were able to ship things to a worker through the same API, then having the arguments might be advantageous (@developit please correct me if I butchered that). But I think that might be too far off to consider now? A separate API or even putting the args in the options bag might work for that case.

[tophf]

Passing the arguments eliminates the need to create a new function so it's good for hot code that posts a lot of tasks.

[shaseley]

While I think we want to be sensitive to overhead concerns, I think some small amount of overhead could be worth the future-proofing, and I think we could always add an args value to the PostTaskOptions dictionary if we proved it was worth it. We're still trying to figure out what to do with the API shape for some follow-up work (yield and task context), and passing something to postTask's callback is a leading option, which makes me favor removing the arg passing for now.

Also, we'd want to understand this better:

1. I’m wondering how common this situation you described would be. Do you have a concrete use case in mind? My understanding is that folks are hesitant to schedule the number of tasks where this would be noticeable just given the general overhead of JS --> Blink (for Chromium).
2. There are other sources of overhead involved with passing the arguments, like (in Chromium) transferring the args from JS to Blink through the bindings layer, and I don’t think it’s obvious which would be faster and by how much. I ran a quick perf test (details in this doc), and I see evidence that the closure method might actually be faster (although a more rigorous analysis would probably be needed).

[esprehn]

@shaseley my intuition is that it should be faster to use a closure as well, since that avoids the ScriptWrappable and v8::Persistent handle creation overhead. Those are also created on the v8 heap just like the Function instance. Tracking all the gc roots through Oilpan is also not free, so letting the arguments be retained by the Function in v8's heap should be cheaper from a GC cost perspective.

Note that a single object like the Function for the closure is very cheap when compared to what happens inside postTask. v8 makes objects for all kinds of things so it's pretty hard to avoid allocations anyway (ex. even floating point numbers are boxed in many cases)

[tophf]

Do you have a concrete use case in mind?

No. Passing params just seemed intentionally similar to setTimeout and setInterval so I assumed this feature is good for performance. Maybe it was 20 years ago but not now?

I ran a quick perf test (details in this doc), and I see evidence that the closure method might actually be faster (although a more rigorous analysis would probably be needed).

Just in case, my concern was the initial overhead to create a different closure each time, not the repeated invocation of the same one.

[shaseley]

Do you have a concrete use case in mind?

No. Passing params just seemed intentionally similar to setTimeout and setInterval so I assumed this feature is good for performance. Maybe it was 20 years ago but not now?

Not sure. Like @esprehn mentioned, arrow functions weren't an option then, so maybe it was more about ergonomics and functionality? But now designing with arrow functions in mind, I think removing the args makes sense.

I ran a quick perf test (details in this doc), and I see evidence that the closure method might actually be faster (although a more rigorous analysis would probably be needed).

Just in case, my concern was the initial overhead to create a different closure each time, not the repeated invocation of the same one.

Ack. In the perf tests, I called scheduler.postTask(() => {foo(a, b, c...)}) in a loop, so it should have created a new object each time. Not sure if V8 does any magic there, but I quickly reran the pass-5-strings test by passing the loop variable too, and the trend was similar. There's definitely noise in the results from my setup, but trends match the intuition around V8 <--> Blink overhead.

[tophf]

Other quite minor advantages of native calls via the [now removed] additional parameters:

1. A simplified call stack in errors, console traces, debugger panels.
2. Line-by-line execution in debugger via step-into hotkey/button won't distract you by entering the dummy arrow function.

There's a workaround for both of these problems though: using bind to create a bound function.