Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Promise.resolve().then(...) to schedule coroutines by default for all platforms (including node.js) with fallback to setTimeout #820

Closed
bashor opened this issue Nov 12, 2018 · 7 comments
Assignees

Comments

@bashor
Copy link
Member

bashor commented Nov 12, 2018

The related micro-benchmark: https://esbench.com/bench/55d4d44e2efbca1100bf7251

Maybe it's worth to have a special implementation for Node.js using process.nextTick.

The related issue: #194

@SeekDaSky
Copy link

SeekDaSky commented Nov 12, 2018

I agree this could lead to a performance improvement (especially on Node.js) but using process.nextTick might not be a proper solution as it means that coroutines will have priority over any event of the event loop, so if too many coroutines are running, no event will be processed.

I made some micro-bench with the same library that power esbench.com and those where the results on my machine (Node.js v11):

setTimeout x 1,120,189 ops/sec ±2.95% (75 runs sampled)
setImmediate x 1,325,972 ops/sec ±2.55% (75 runs sampled)
process.NextTick x 2,623,686 ops/sec ±2.08% (76 runs sampled)
Promise.resolve x 1,831,654 ops/sec ±4.65% (67 runs sampled)

In the light of the differences in performances I think changing the Node.js dispatcher implementation is something to consider.

I uploaded the bench if you are interested bench.zip

@bashor
Copy link
Member Author

bashor commented Nov 28, 2018

Another reason to do it (ASAP?): #863

@qwwdfsad qwwdfsad self-assigned this Feb 27, 2019
@qwwdfsad
Copy link
Collaborator

qwwdfsad commented Feb 28, 2019

I am afraid we can't use Promise.resolve by default.

The problem (and the virtue) of promises lies in the fact that they use different scheduling mechanism aka microtasking. And microtasks queue has the priority over regular tasks (e.g. ready-to-run setTimeout tasks or animation tasks).

I am concerned that with microtasking as default dispatch mechanism, any user can easily shoot himself in the foot.

For example, do you expect the following snippet to eventually return null?

withTimeoutOrNull(100) {  // <- setTimeout({ thisBlock.cancel() }, 100)
    while (true) {
      yield() // <- Promise.resolve().then(thisBlock)
    }
} 

If microtasking is used for dispatching, this code may or may not complete, depending on the JS engine. E.g. in node on my local machine it hangs. In the chrome this block completes with null successfully.

@SeekDaSky
Copy link

Maybe having setTimeout(0) as primary dispatcher and Promise.resolve as the immediate dispatcher would work out? with this the developer could choose when he wants a faster dispatching, knowing the risks of bypassing the regular event loop.

@qwwdfsad
Copy link
Collaborator

qwwdfsad commented Feb 28, 2019

This idea sounds good and I was thinking about it as well.
Though I see at least one problem with that: immediate dispatch will behave differently on different platforms, and moreover, on different JS engines:

println(1)
launch(Dispatchers.Main.immediate) {
  println(2)
}
println(3)

will print 1 2 3 on JVM and 1 3 2 on JS. Note that the problem is not the order of invocation, but completely different semantics.

What we can do though is to provide another JS-specific property, e.g. Dispatchers.Main.foo that uses resolve as dispatch mechanism. The only question are how to name it

@qwwdfsad qwwdfsad added the js label Feb 28, 2019
@SeekDaSky
Copy link

Good point, the fact that different JS engines behave differently is annoying but JS devs are (unfortunately) used to this kind of things.
Using another name sounds good, though I have no idea how to name it. Maybe withResolve or promiseBased, but that looks a bit verbose.

@qwwdfsad
Copy link
Collaborator

Fixed in 1.3.0-M1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants