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

Multithreading #8112

Merged
merged 51 commits into from Sep 2, 2019

Conversation

@waj
Copy link
Member

commented Aug 23, 2019

This is a big PR that adds initial and experimental support for multithreading. It works side by side with the current single thread implementation and must be enabled with the preview_mt flag.

There is an upcoming blog post explaining details about the implementation and decisions made on this work.

Things covered by this PR:

  • One scheduler per thread (workers): each thread has a queue of runnable fibers. Each fiber is assigned to a worker when it starts and currently it lives in the same worker until it finishes (no job stealing)
  • One event loop per thread: whenever a thread becomes idle (no runnable fibers left) it waits on the event loop for events (IO, timers). The same event loop also receives notifications about new fibers created or awaken from other threads.
  • New implementation of a thread safe Channel: re-created from scratch because with the previous implementation it was harder to make a multi thread select operation.
  • Atomic initialization of constants and class variables: to avoid race conditions
  • Thread safe Mutex

Things not covered by this PR:

  • Job stealing: naive approaches to implement this are detrimental to performance
  • Soundness of types: union types might become unsound in MT mode
  • Thread safety of std classes: for example, Array is not thread safe and must be protected by a mutex in case it's shared between fibers
waj and others added 30 commits Aug 13, 2019
Move event_loop ivars to Thread
Use libevent pthread support
…. Added main thread as a worker.
…ed. It now supports MT for send/receive and select operations.
…lation is fixed). Use simpler but effective RR scheduling instead.
… select statement
…nqueue_self as Scheduler#enqueue now always sends the fiber to the local queue.
…ode.
…dcoded value (otherwise the constructor of Time requires the value of UNIX_EPOCH already set to validate the parameters)
…ltithreading)
…r thread
Only positive integer values are accepted or empty string (that switches back to the default number of workers). Any other value will make the program to crash.
@asterite

This comment has been minimized.

Copy link
Member

commented on src/crystal/event_loop.cr in fbbefd1 Aug 26, 2019

This won't compile because dns_base lacks a type. However it currently compiles because it seems to be unused. I see it being called from create_dns_request but that method isn't called anywhere. Should we remove this or did we forget to do something? Maybe we used it before but we stopped using it because libevent didn't work well inside Docker containers or something like that...

This comment has been minimized.

Copy link
Member Author

replied Aug 26, 2019

Good catch!! Actually that method is not used for a loooong time, since this commit: 56dec21

src/crystal/scheduler.cr Outdated Show resolved Hide resolved
src/crystal/scheduler.cr Show resolved Hide resolved
src/thread.cr Outdated Show resolved Hide resolved
src/fiber.cr Outdated Show resolved Hide resolved
src/crystal/thread_local_value.cr Outdated Show resolved Hide resolved
src/compiler/crystal/codegen/once.cr Outdated Show resolved Hide resolved
src/crystal/spin_lock.cr Show resolved Hide resolved
src/crystal/spin_lock.cr Show resolved Hide resolved
src/process.cr Show resolved Hide resolved
waj and others added 8 commits Aug 26, 2019
… and enable it only in multithread mode
@asterite

This comment has been minimized.

Copy link
Member

commented on src/crystal/fiber_channel.cr in 483d945 Aug 27, 2019

@waj I wonder if performance changes if we change these to be IO::FileDescriptor instead of IO. That way there's no need for a virtual/multidispatch. Just something to try :-)

This comment has been minimized.

Copy link
Member Author

replied Aug 27, 2019

It does! It's obviously really small difference but everything counts. Thanks!

This comment has been minimized.

Copy link
Member Author

replied Aug 27, 2019

I just tested with a multithread matrix multiplication example that heavily exercises inter thread communication without any IO involved and the difference is around 1% improvement.

This comment has been minimized.

Copy link
Member

replied Aug 27, 2019

Cool! 🎉

private def self.event_base
Thread.current.event_base
end

private def self.loop_fiber

This comment has been minimized.

Copy link
@j8r

j8r Aug 27, 2019

Contributor

Not used anywhere.

This comment has been minimized.

Copy link
@waj

waj Aug 27, 2019

Author Member

Hmm... that's not true. It's used in many places.

This comment has been minimized.

Copy link
@j8r

j8r Aug 27, 2019

Contributor

I didn't found it in this PR nor with grep -rn in master.

This comment has been minimized.

Copy link
@j8r

j8r Aug 27, 2019

Contributor

I'm talking about this very private class method, not the instance method and variable.

This comment has been minimized.

Copy link
@waj

waj Aug 27, 2019

Author Member

The instance variable belongs to Thread. This static method is defined in Crystal::EventLoop module.

This comment has been minimized.

Copy link
@j8r

j8r Aug 27, 2019

Contributor

Sure, but sorry I didn't undestand the code URL you mentioned, where EventLoop.loop_fiber is used then?

This comment has been minimized.

Copy link
@asterite

asterite Aug 28, 2019

Member

Same file, line 26

loop_fiber.resume

This comment has been minimized.

Copy link
@j8r

j8r Aug 28, 2019

Contributor

Oh ok sorry, thanks. I didn't know this language feature, to call a static method like that.

@asterite asterite referenced this pull request Aug 27, 2019
12 of 21 tasks complete
module Crystal::EventLoop
{% unless flag?(:preview_mt) %}
def self.after_fork
Thread.current.event_base.reinit

This comment has been minimized.

Copy link
@j8r

j8r Aug 28, 2019

Contributor
Suggested change
Thread.current.event_base.reinit
event_base.reinit
Copy link
Contributor

left a comment

This looks fantastic! I'm excited about how much thought has been given to making it easy on application developers to get parallel execution while mitigating thread-safety issues!

src/crystal/rw_lock.cr Show resolved Hide resolved
@@ -58,8 +58,11 @@ end
#
# 2.times { ch.receive }
# ```
def spawn(*, name : String? = nil, &block)
def spawn(*, name : String? = nil, same_thread = false, &block)

This comment has been minimized.

Copy link
@jgaskins

jgaskins Aug 28, 2019

Contributor

Such a small thing but sooooo crucial. I use spawn all the time to move work out-of-band but I rely on it waiting until the current fiber is yielded.

Something that's been in the back of my mind every time I've done it is "this isn't going to work when Crystal goes multithreaded" but this one argument, along with fibers staying within their threads, solves that problem! 💯 🙌🏼

This comment has been minimized.

Copy link
@jhass

jhass Aug 28, 2019

Member

Just make sure nobody changes the contract from "stays on the same thread" to "is initially queued for the same thread" in case job stealing is implemented :D

This comment has been minimized.

Copy link
@bcardiff

bcardiff Aug 28, 2019

Member

@jgaskins I would recommend using Channel to wait for the result. The same_thread is more to ensure the execution context than the scheduling.

This comment has been minimized.

Copy link
@j8r

j8r Aug 28, 2019

Contributor

I just notice that same_thread should be typed as Bool here

This comment has been minimized.

Copy link
@asterite

asterite Aug 28, 2019

Member

@j8r Type restrictions are optional in Crystal. In this way one can pass a nilable thing and it will work. Restricting it to Bool will make it less flexible, forcing you to do things like !!exp.

I think we should embrace the way Crystal lets you omit type annotations. If something is not strictly required by the language then we shouldn't always push for it.

@crystal-lang crystal-lang deleted a comment Aug 28, 2019
@crystal-lang crystal-lang deleted a comment Aug 28, 2019
@crystal-lang crystal-lang deleted a comment Aug 28, 2019
Copy link
Member

left a comment

:shipit: :shipit:

@waj waj merged commit f73c4bb into crystal-lang:master Sep 2, 2019
6 checks passed
6 checks passed
ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
ci/circleci: test_preview_mt Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.