[core] Use an actor model for tile worker concurrency #6337

jfirebaugh · 2016-09-15T00:21:37Z

GeometryTile and RasterTile now communicate with their asynchronous worker according to the actor model.

This has the following benefits:

Source-specific processing lives in source-specific worker classes (GeometryTileWorker and RasterTileWorker).
No need to spawn a new set of worker threads when the style changes. We could switch to a process-wide worker pool easily.
No need to write Worker wrappers for every TileWorker method.
Impossible to access worker state from the tile object.
"Invocation" is now 100% fire-and-forget: there is no support for cancelling a previous message and thus no possibility of unexpectedly blocking on cancellation.
Introduces a more flexible model for asynchronous messaging, making Remove concurrent access to objects from worker threads #667 more tractable.
Provides a clear path for using the built-in thread pool on node (worker usage should be customizable #3982).
Fixes Worker pool should share queue instead of round-robin distribution #1471.
Fixes Move WorkTask callback invocation after task invocation #3722.
Fixes Fix race condition with GeometryTile::redoLayout #6263 and removes the band-aids ([core] Band-aid to prevent GeometryTile::redoLayout race condition from causing a crash #6322, [core] Another band-aid (#6322) #6333).

Code docs

An Actor<O> is an owning reference to an asynchronous object of type O: an "actor".
Communication with an actor happens via message passing: you send a message to the object
(using invoke), passing a pointer to the member function to call and arguments which
are then forwarded to the actor.

The actor receives messages sent to it asynchronously, in a manner defined its Scheduler.
To store incoming messages before their receipt, each actor has a Mailbox, which acts as
a FIFO queue. Messages sent from actor S to actor R are guaranteed to be processed in the
order sent. However, relative order of messages sent by two different actors S1 and S2
to R is not guaranteed (and can't be: S1 and S2 may be acting asynchronously with respect
to each other).

Construction and destruction of an actor is currently synchronous: the corresponding O
object is constructed synchronously by the Actor constructor, and destructed synchronously
by the ~Actor destructor, after ensuring that the O is not currently receiving an
asynchronous message. (Construction and destruction may change to be asynchronous in the
future.)

An Actor<O> can be converted to an ActorRef<O>, a non-owning value object representing
a (weak) reference to the actor. Messages can be sent via the Ref as well.

It's safe -- and encouraged -- to pass Refs between actors via messages. This is how two-way
communication and other forms of collaboration between multiple actors is accomplished.

It's safe for a Ref to outlive its Actor -- the reference is "weak", and does not extend
the lifetime of the owning Actor, and sending a message to a Ref whose Actor has died is
a no-op. (In the future, a dead-letters queue or log may be implemented.)

Please don't send messages that contain shared pointers or references. That subverts the
purpose of the actor model: prohibiting direct concurrent access to shared state.

A Scheduler is responsible for coordinating the processing of messages by
one or more actors via their mailboxes. It's an abstract interface. Currently,
the following concrete implementations exist:

ThreadPool can coordinate an unlimited number of actors over any number of
threads via a pool, preserving the following behaviors:
- Messages from each individual mailbox are processed in order
- Only a single message from a mailbox is processed at a time; there is no
  concurrency within a mailbox
Subject to these constraints, processing can happen on whatever thread in the
pool is available.
RunLoop is a Scheduler that is typically used to create a mailbox and
ActorRef for an object that lives on the main thread and is not itself wrapped
as an Actor:
```
  auto mailbox = std::make_shared<Mailbox>(*util::RunLoop::Get());
  Actor<Worker> worker(threadPool, ActorRef<Foo>(*this, mailbox));
```

Notes on implementation patterns

The actor model is a constrained environment. All you can do is send messages! Implementing behaviors that would be easy in a single threaded or shared memory environment takes a degree of cleverness, or at least knowledge of certain patterns.

Here are the patterns that GeometryTile / GeometryTileWorker use to get their job done:

Correlation Identifier allows GeometryTile to know when the worker has finished processing the most recent data it's been sent, such that the tile can be considered "fully loaded".
GeometryTileWorker is a State Machine: it transitions between states depending on which messages it receives, and its behavior for messages of a certain type depends on what state it's in.
GeometryTileWorker creates a Self-Sent Message: a message it sends to itself, asynchronously. Its state transitions are defined in such a way that this allows it to coalesce the other messages it receives, to avoid doing obsolete work.

If we use actors more, which I think we should, we'll probably reuse these patterns and more.

TODO

Now:

Write unit tests for the new types. They're integration tested via existing tests, but should be independently tested.

Future:

Convert Thread users to actors, and remove Thread. This is a strictly better model.
Think about adding a "supervisor" concept/type and more clearly defined exception handling behavior.

mention-bot · 2016-09-15T00:21:39Z

@jfirebaugh, thanks for your PR! By analyzing this pull request, we identified @ansis, @tmpsantos and @mikemorris to be potential reviewers.

kkaefer · 2016-09-15T15:14:56Z

\o/

jfirebaugh · 2016-09-15T18:42:32Z

Added unit tests and did some high-intensity panning, zooming, rotating, and style switching on my iPhone. Everything is looking good.

@tmpsantos, @kkaefer, want to review?

tmpsantos

Good job with this refactoring.

tmpsantos · 2016-09-15T18:09:44Z

src/mbgl/actor/thread_pool.cpp

+void ThreadPool::schedule(std::weak_ptr<Mailbox> mailbox) {
+    std::lock_guard<std::mutex> lock(mutex);
+    queue.push(mailbox);
+    cv.notify_one();


Is this safe? If all threads are busy will this cause the message on the mailbox to starve because no thread will be waiting on the synchronization point?

It's safe because we use the version of condition_variable::wait which accepts a predicate. If all threads are busy, on the subsequent loop whichever thread obtains the mutex first will check the predicate !queue.empty() || terminate.load(), see that it's true, and not wait on the condition variable.

tmpsantos · 2016-09-15T18:16:12Z

src/mbgl/actor/thread_pool.cpp

+                queue.pop();
+                lock.unlock();
+
+                if (auto locked = mailbox.lock()) {


I particularly find using a = b inside an if confusing as I tend to think it was a typo.

This is the idiomatic use of weak_ptr; see e.g. here. In general, declaring on a separate line would require an extra set of braces and indentation to preserve the same region of locking:

{ auto locked = mailbox.lock(); if (locked) { ... } }

tmpsantos · 2016-09-15T18:21:31Z

src/mbgl/map/map.cpp

@@ -99,6 +101,7 @@ Map::Impl::Impl(View& view_,
      contextMode(contextMode_),
      pixelRatio(view.getPixelRatio()),
      asyncUpdate([this] { update(); }),
+      workerThreadPool(4),


Maybe we should move the number of works to constants.hpp

Agreed. I think we can target that for a followup that also makes the thread pool global, instead of per-Map.

tmpsantos · 2016-09-15T18:30:05Z

include/mbgl/util/run_loop.hpp

@@ -78,6 +81,14 @@ class RunLoop : private util::noncopyable {

    void push(std::shared_ptr<WorkTask>);

+    void schedule(std::weak_ptr<Mailbox> mailbox) override {


Nice, so when we get rid of util::Thread we can get rid of all the locking and complexity of the RunLoop, I suppose.

tmpsantos · 2016-09-15T18:33:29Z

src/mbgl/actor/scheduler.hpp

+class Mailbox;
+
+/*
+    A `Scheduler` is responsible for coordinating the processing of messages by


I love this abstraction. It could be used for creating a out-of-process worker if ever needed.

tmpsantos · 2016-09-15T20:37:54Z

src/mbgl/tile/raster_tile_worker.cpp

+
+namespace mbgl {
+
+RasterTileWorker::RasterTileWorker(ActorRef<RasterTileWorker>, ActorRef<RasterTile> parent_)


It is not clear to me what is the first parameter on this constructor used for.

Good point, I'll document this.

tmpsantos

@kkaefer can you also 👀 before we merge?

jfirebaugh force-pushed the worker-actor branch 3 times, most recently from 54e81ca to 13f7484 Compare September 15, 2016 01:23

jfirebaugh force-pushed the worker-actor branch 3 times, most recently from 96b49ca to 5468f86 Compare September 15, 2016 18:20

tmpsantos reviewed Sep 15, 2016

View reviewed changes

tmpsantos approved these changes Sep 15, 2016

View reviewed changes

jfirebaugh mentioned this pull request Sep 15, 2016

Refactor "Worker", "Dispatcher", "Actor" into a general purpose "PooledWorker" utility mapbox/mapbox-gl-js#3034

Open

jfirebaugh force-pushed the worker-actor branch from a069a0e to d8f9489 Compare September 16, 2016 16:02

jfirebaugh added 2 commits September 16, 2016 12:01

[core] Use an actor model for tile worker concurrency

41bbd4e

[core] Document the ActorRef<O> argument passed to O's constructor

d2246b2

jfirebaugh force-pushed the worker-actor branch from d8f9489 to d2246b2 Compare September 16, 2016 19:01

jfirebaugh merged commit d2246b2 into master Sep 16, 2016

jfirebaugh deleted the worker-actor branch September 16, 2016 19:01

jfirebaugh mentioned this pull request Sep 16, 2016

~Actor<O> is blocking #2644

Closed

tmpsantos mentioned this pull request Sep 22, 2016

[core] Remove util::Thread #6425

Closed

4 tasks

kkaefer mentioned this pull request Jan 11, 2017

Convert DefaultFileSource and dependents to actor model #7678

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Use an actor model for tile worker concurrency #6337

[core] Use an actor model for tile worker concurrency #6337

jfirebaugh commented Sep 15, 2016 •

edited

Loading

mention-bot commented Sep 15, 2016

kkaefer commented Sep 15, 2016

jfirebaugh commented Sep 15, 2016

tmpsantos left a comment

tmpsantos Sep 15, 2016

jfirebaugh Sep 15, 2016

tmpsantos Sep 15, 2016 •

edited

Loading

jfirebaugh Sep 15, 2016

tmpsantos Sep 15, 2016

jfirebaugh Sep 15, 2016

tmpsantos Sep 15, 2016

tmpsantos Sep 15, 2016 •

edited

Loading

tmpsantos Sep 15, 2016

jfirebaugh Sep 15, 2016

tmpsantos left a comment

		@@ -78,6 +81,14 @@ class RunLoop : private util::noncopyable {

		void push(std::shared_ptr<WorkTask>);

		void schedule(std::weak_ptr<Mailbox> mailbox) override {


		namespace mbgl {

		RasterTileWorker::RasterTileWorker(ActorRef<RasterTileWorker>, ActorRef<RasterTile> parent_)

[core] Use an actor model for tile worker concurrency #6337

[core] Use an actor model for tile worker concurrency #6337

Conversation

jfirebaugh commented Sep 15, 2016 • edited Loading

Code docs

Notes on implementation patterns

TODO

mention-bot commented Sep 15, 2016

kkaefer commented Sep 15, 2016

jfirebaugh commented Sep 15, 2016

tmpsantos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmpsantos Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmpsantos Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmpsantos left a comment

Choose a reason for hiding this comment

jfirebaugh commented Sep 15, 2016 •

edited

Loading

tmpsantos Sep 15, 2016 •

edited

Loading

tmpsantos Sep 15, 2016 •

edited

Loading