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

[PoC] Promise based coroutines #36

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

arthurschreiber
Copy link
Contributor

This adds Promise based coroutines, inspired by Bluebird's Promise.coroutine and JavaScript's async/await, but based on Ruby's Fibers.

Example:

p1 = Promise.new
p2 = Promise.coroutine do
  Promise.await(p1) + 1
end

p1.pending? # => true
p2.pending? # => true

p1.fulfill(10)
p2.fulfilled? #=> true
p2.value #=> 11

begin...rescue...end also works:

p1 = Promise.new
p2 = Promise.coroutine do
  begin
    Promise.await(p1)
  rescue => e
    e.message
  end
end

p1.pending? # => true
p2.pending? # => true

p1.reject(RuntimeError.new("fail"))
p2.fulfilled? #=> true
p2.value #=> "fail"

What do you think? If this is something that would be nice to have in promise.rb, I'll add tests and API documentation. 😄

/cc @rmosolgo @josh

@arthurschreiber
Copy link
Contributor Author

Performance-wise, this is "pretty okay":

require "benchmark/ips"
require "promise"

Benchmark.ips do |x|
  x.report("coroutine/await") do
    p1 = Promise.new
    p2 = Promise.new
    p3 = Promise.new
    p4 = Promise.new

    promise = Promise.coroutine do
      Promise.await p1
      Promise.await Promise.coroutine {
        p2
      }
      Promise.await p3
      Promise.await p4
    end

    p1.fulfill(1)
    p2.fulfill(2)
    p3.fulfill(3)
    p4.fulfill(4)
  end

  x.report("chained") do
    p1 = Promise.new
    p2 = Promise.new
    p3 = Promise.new
    p4 = Promise.new

    promise = p1.then { p2.then }.then { p3 }.then { p4 }

    p1.fulfill(1)
    p2.fulfill(2)
    p3.fulfill(3)
    p4.fulfill(4)
  end

  x.compare!
end
Warming up --------------------------------------
     coroutine/await     5.806k i/100ms
             chained     7.005k i/100ms
Calculating -------------------------------------
     coroutine/await     62.228k (± 8.7%) i/s -    313.524k in   5.076862s
             chained     78.868k (± 4.5%) i/s -    399.285k in   5.073268s

Comparison:
             chained:    78868.2 i/s
     coroutine/await:    62227.9 i/s - 1.27x  slower

@dylanahsmith
Copy link
Collaborator

The PR description uses Promise.await which you don't have in the code, although I can see how that would work.

Performance-wise, this is "pretty okay"

I thought the main problem with fibers was the fixed stack size. As in large fibers could result in overflows from using a smaller heap than the thread's stack and small fibers would waste memory. If you are planning on using it with graphql-batch, then I would expect they would end up being small fibers and would use a lot more memory.

What do you think? If this is something that would be nice to have in promise.rb

I'm hesitant to add something that won't be widely used, yet would still need to be maintained for backwards compatibility. Also, it looks like something that can exist outside of this gem so it can be tried out in production.

@arthurschreiber
Copy link
Contributor Author

The PR description uses Promise.await which you don't have in the code, although I can see how that would work.

Whoops, yea, I somehow missed adding that. I added it now. 😅

I thought the main problem with fibers was the fixed stack size.

This was true in Ruby 1.9, but it should no longer be an issue in Ruby 2.0, where the default fiber stack size was increased considerably:
https://github.com/ruby/ruby/blob/073cc5e815fcf5178fe4e515fcde74dc3597adeb/ChangeLog#L7159-L7185. There might still be an issue with having a large number of fibers existing at the same time, but I haven't tested this yet on a real world application.

I'm hesitant to add something that won't be widely used, yet would still need to be maintained for backwards compatibility. Also, it looks like something that can exist outside of this gem so it can be tried out in production.

That is totally understandable! 👍 I also would not want this to be merged without having done some proper performance and feasibility testing. This is just something I've been toying around recently and wanted it to throw out there for other people to see and to get some feedback. 😄

* coroutine functionality needs to be explicitly required by loading
`promise/coroutine`
* Promises returned by `Promise.coroutine` can now be correctly
`.sync`ed.
* The block passed to `Promise.coroutine` is no longer converted to a
proc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants