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

ThreadPool cleanup (3/n): Switch to vectorized API & remove unused/co… #32

Merged
merged 6 commits into from
May 24, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,40 @@ public class NonBlockingThreadPool<Environment: ConcurrencyPlatform>: ComputeThr
if let e = err { throw e }
}

public func parallelFor(n: Int, _ fn: VectorizedParallelForFunction) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc comment please!!!

Unless "parallel for on a thread pool" is a very well-established concept, I'd consider renaming these. for in Swift is something we do over a Sequence, and there's no sequence here. Is this something that could be written as an extension on Collection that accepts a thread pool as an argument? Your n repetitions could be well-represented by 0..<n.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to doc comment. PTAL?

I believe that this should most often be accessed as an operation on a random access (or likely some form of "splittable") collection. But in any case, that will have to be generic over the thread pool itself, so we don't get away from having this method and coming up with a name for it.

Note: I started going in this direction a while back but I think that direction needs a "reboot". For now, I'd like to focus on getting this low-level API implemented correctly and efficiently, and we can then refactor and/or stack on the further abstractions.

FWIW: I started out by having VectorizedParallelForFunction take a range instead of 2 integers representing the start and end, but that makes type inference not work as well (as code requires annotations because the alternative API induces an ambiguity between the non-vectorized and vectorized APIs).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Will comment on the doc comment separately (GitHub doesn't make this super convenient)
  • All collections are "splittable" for reasonable definitions of the term, but maybe you mean collections whose disjoint slices can be mutated in parallel. The more general concept is those that “have disjoint parts that can be projected for mutation in parallel.” You could imagine a collection of pairs where you mutate the first of each pair in one thread and the second in another.
  • Not quite sure what you wanted me to notice at that link. The main thing I took away was, “why is he rebasing those slices?“ which probably wasn't the point 😉
  • IMO it's questionable whether we really want the non-vectorized ones and whether they should have the same spelling anyway.

let grainSize = n / parallelism // TODO: Make adaptive!

func executeParallelFor(_ start: Int, _ end: Int) {
if start + grainSize >= end {
fn(start, end, n)
} else {
// Divide into 2 & recurse.
let rangeSize = end - start
let midPoint = start + (rangeSize / 2)
self.join({ executeParallelFor(start, midPoint) }, { executeParallelFor(midPoint, end)})
}
}

executeParallelFor(0, n)
}

public func parallelFor(n: Int, _ fn: ThrowingVectorizedParallelForFunction) throws {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc comment please!

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1; done. (Although I suspect that this comment could be improved...)

let grainSize = n / parallelism // TODO: Make adaptive!

func executeParallelFor(_ start: Int, _ end: Int) throws {
if start + grainSize >= end {
try fn(start, end, n)
} else {
// Divide into 2 & recurse.
let rangeSize = end - start
let midPoint = start + (rangeSize / 2)
try self.join({ try executeParallelFor(start, midPoint) }, { try executeParallelFor(midPoint, end) })
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? Your change description gives the impression you are removing the implementation of parallelFor in terms of join, yet here it is.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good point. That description is getting ahead of the actual implementation in this patch set. I'll update the description in the PR shortly.

}
}

try executeParallelFor(0, n)
}

/// Shuts down the thread pool.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meaningful summary please. What does it mean to shut a thread pool down?

public func shutDown() {
cancelled = true
Expand Down
172 changes: 76 additions & 96 deletions Sources/PenguinParallel/ThreadPool.swift
Original file line number Diff line number Diff line change
Expand Up @@ -60,25 +60,61 @@ public protocol ComputeThreadPool {
/// This is the throwing overload
func join(_ a: () throws -> Void, _ b: () throws -> Void) throws

/// A function that can be executed in parallel.
///
/// The first argument is the index of the invocation, and the second argument is the total number
/// of invocations.
typealias ParallelForFunction = (Int, Int) -> Void
saeta marked this conversation as resolved.
Show resolved Hide resolved

/// A function that can be executed in parallel.
///
/// The first argument is the index of the copy, and the second argument is the total number of
/// copies being executed.
typealias ParallelForFunc = (Int, Int) throws -> Void
typealias ThrowingParallelForFunction = (Int, Int) throws -> Void

/// A vectorized function that can be executed in parallel.
///
/// The first argument is the start index for the vectorized operation, and the second argument
/// corresponds to the end of the range. The third argument contains the total size of the range.
typealias VectorizedParallelForFunction = (Int, Int, Int) -> Void

/// A vectorized function that can be executed in parallel.
///
/// The first argument is the start index for the vectorized operation, and the second argument
/// corresponds to the end of the range. The third argument contains the total size of the range.
typealias ThrowingVectorizedParallelForFunction = (Int, Int, Int) throws -> Void

/// Returns after executing `fn` `n` times.
///
/// - Parameter n: The total times to execute `fn`.
func parallelFor(n: Int, _ fn: ParallelForFunc) rethrows
func parallelFor(n: Int, _ fn: ParallelForFunction)

/// Returns after executing `fn` an unspecified number of times, guaranteeing that `fn` has been
/// called with parameters that perfectly cover of the range `0..<n`.
///
/// - Parameter n: The range of numbers `0..<n` to cover.
func parallelFor(n: Int, _ fn: VectorizedParallelForFunction)

/// Returns after executing `fn` `n` times.
///
/// - Parameter n: The total times to execute `fn`.
func parallelFor(n: Int, _ fn: ThrowingParallelForFunction) throws

/// Returns after executing `fn` an unspecified number of times, guaranteeing that `fn` has been
/// called with parameters that perfectly cover of the range `0..<n`.
///
/// - Parameter n: The range of numbers `0..<n` to cover.
func parallelFor(n: Int, _ fn: ThrowingVectorizedParallelForFunction) throws


// TODO: Add this & a default implementation!
// /// Returns after executing `fn` `n` times.
// ///
// /// - Parameter n: The total times to execute `fn`.
// /// - Parameter blocksPerThread: The minimum block size to subdivide. If unspecified, a good
// /// value will be chosen based on the amount of available parallelism.
// func parallelFor(blockingUpTo n: Int, blocksPerThread: Int, _ fn: ParallelForFunc)
// func parallelFor(blockingUpTo n: Int, _ fn: ParallelForFunc)
// func parallelFor(blockingUpTo n: Int, blocksPerThread: Int, _ fn: ParallelForFunction)
// func parallelFor(blockingUpTo n: Int, _ fn: ParallelForFunction)

/// The maximum amount of parallelism possible within this thread pool.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of redundant with the name, yeah?
Please find a description that doesn't beg the question, "what does it mean to have a maximum parallelism of N?“ from the point-of-view of someone who just has the threadpool abstraction to work with.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a quick pass, although this can probably be refined further.

var parallelism: Int { get }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var parallelism: Int { get }
var maxParallelism: Int { get }

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol had a similar thought after pondering the doc comment a bit further. 👍

Expand All @@ -91,105 +127,31 @@ public protocol ComputeThreadPool {
}

extension ComputeThreadPool {
/// A default implementation of the non-throwing variation in terms of the throwing one.
public func join(_ a: () -> Void, _ b: () -> Void) {
withoutActuallyEscaping(a) { a in
let throwing: () throws -> Void = a
try! join(throwing, b)
}
}
}

/// Holds a parallel for function; this is used to avoid extra refcount overheads on the function
/// itself.
fileprivate struct ParallelForFunctionHolder {
var fn: ComputeThreadPool.ParallelForFunc
}

/// Uses `ComputeThreadPool.join` to execute `fn` in parallel.
fileprivate func runParallelFor<C: ComputeThreadPool>(
pool: C,
start: Int,
end: Int,
total: Int,
fn: UnsafePointer<ParallelForFunctionHolder>
) throws {
if start + 1 == end {
try fn.pointee.fn(start, total)
} else {
assert(end > start)
let distance = end - start
let midpoint = start + (distance / 2)
try pool.join(
{ try runParallelFor(pool: pool, start: start, end: midpoint, total: total, fn: fn) },
{ try runParallelFor(pool: pool, start: midpoint, end: end, total: total, fn: fn) })
}
}

extension ComputeThreadPool {
public func parallelFor(n: Int, _ fn: ParallelForFunc) rethrows {
try withoutActuallyEscaping(fn) { fn in
var holder = ParallelForFunctionHolder(fn: fn)
try withUnsafePointer(to: &holder) { holder in
try runParallelFor(pool: self, start: 0, end: n, total: n, fn: holder)
/// Convert a non-vectorized operation to a vectorized operation.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This summary makes no sense to me. I'd want it to read “Converts…” but then a function that converts one thing to another thing returns that other thing. Looking further, the doc comment appears to be a description of the implementation technique, not of what the function does.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I thought that comments on extension methods that are implementations of methods on the protocols themselves don't show up in typical doc-generation, I tried to write something different & more specific here. I can certainly just copy-pasta the doc comment from the protocol method itself if you think that's more appropriate... :-)

That said, I've attempted to refine this a bit (in the same direction, however).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I thought that comments on extension methods that are implementations of methods on the protocols themselves don't show up in typical doc-generation

You are mistaken:

image

Copy 🍝 is always delicious though somewhat unoriginal. Doc comments are for the user of the API. If you want to write something for maintainers, use //.

public func parallelFor(n: Int, _ fn: ParallelForFunction) {
parallelFor(n: n) { start, end, total in
for i in start..<end {
fn(i, total)
}
}
}
}

/// Typed compute threadpools support additional sophisticated operations.
public protocol TypedComputeThreadPool: ComputeThreadPool {
/// Submit a task to be executed on the threadpool.
///
/// `pRun` will execute task in parallel on the threadpool and it will complete at a future time.
/// `pRun` returns immediately.
func dispatch(_ task: (Self) -> Void)

/// Run two tasks (optionally) in parallel.
///
/// Fork-join parallelism allows for efficient work-stealing parallelism. The two non-escaping
/// functions will have finished executing before `pJoin` returns. The first function will execute on
/// the local thread immediately, and the second function will execute on another thread if resources
/// are available, or on the local thread if there are not available other resources.
func join(_ a: (Self) -> Void, _ b: (Self) -> Void)

/// Run two throwing tasks (optionally) in parallel; if one task throws, it is unspecified
/// whether the second task is even started.
///
/// This is the throwing overloaded variation.
func join(_ a: (Self) throws -> Void, _ b: (Self) throws -> Void) throws
}

extension TypedComputeThreadPool {
/// Implement the non-throwing variation in terms of the throwing one.
public func join(_ a: (Self) -> Void, _ b: (Self) -> Void) {
withoutActuallyEscaping(a) { a in
let throwing: (Self) throws -> Void = a
// Implement the non-throwing in terms of the throwing implementation.
try! join(throwing, b)
/// Convert a non-vectorized operation to a vectorized operation.
public func parallelFor(n: Int, _ fn: ThrowingParallelForFunction) throws {
try parallelFor(n: n) { start, end, total in
for i in start..<end {
try fn(i, total)
}
}
}
}

extension TypedComputeThreadPool {
public func dispatch(_ fn: @escaping () -> Void) {
dispatch { _ in fn() }
}

public func join(_ a: () -> Void, _ b: () -> Void) {
join({ _ in a() }, { _ in b() })
}

public func join(_ a: () throws -> Void, _ b: () throws -> Void) throws {
try join({ _ in try a() }, { _ in try b() })
}
}

/// A `ComputeThreadPool` that executes everything immediately on the current thread.
///
/// This threadpool implementation is useful for testing correctness, as well as avoiding context
/// switches when a computation is designed to be parallelized at a coarser level.
public struct InlineComputeThreadPool: TypedComputeThreadPool {
public struct InlineComputeThreadPool: ComputeThreadPool {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public struct InlineComputeThreadPool: ComputeThreadPool {
public struct SerialExecution: ParallelizableExecutionStrategy {

Consider the protocol name an initial suggestion. ComputeThreadPool seems wrong to me, as models are not necessarily thread pools.

When we do mention Threads, is there any point in adding Compute?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of thread-pools, there can be a number of different designs with different properties. In the same way that you can implement a random access collection in terms of a collection (just really inefficiently), I wanted to clearly distinguish what properties the thread-pool has. Concretely, there are I/O-focused thread-pools, where you can blocking and/or non-blocking I/O. This thread pool abstraction is focused on compute-bound tasks, and is tuned / structured with APIs focused on that domain. Does that make sense?

Happy to ponder the names further... related work also uses ConcurrentWorkQueue.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of thread-pools, there can be a number of different designs with different properties.

Naturally.

In the same way that you can implement a random access collection in terms of a collection (just really inefficiently),

You mean like Sampling<SomeCollection, [SomeCollection.Index]>? (Phew, glad I looked at that just now!)

I wanted to clearly distinguish what properties the thread-pool has.

Also naturally. I don't get the connection to random access, though.

Concretely, there are I/O-focused thread-pools, where you can blocking and/or non-blocking I/O. This thread pool abstraction is focused on compute-bound tasks, and is tuned / structured with APIs focused on that domain. Does that make sense?

In principle. Are they really separate abstractions though, or at least, isn't the IO one a refinement of this one? Wouldn't you want to write most algorithms once rather than replicate them for different kinds of pools?

/// Initializes `self`.
public init() {}

Expand All @@ -202,14 +164,32 @@ public struct InlineComputeThreadPool: TypedComputeThreadPool {
/// Dispatch `fn` to be run at some point in the future (immediately).
///
/// Note: this implementation just executes `fn` immediately.
public func dispatch(_ fn: (Self) -> Void) {
fn(self)
public func dispatch(_ fn: () -> Void) {
dabrahams marked this conversation as resolved.
Show resolved Hide resolved
fn()
}

/// Executes `a` and `b` optionally in parallel, and returns when both are complete.
///
/// Note: this implementation simply executes them serially.
public func join(_ a: () -> Void, _ b: () -> Void) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think of join as an operation that waits on the completion of one or more already concurrently-executing things.

Suggested change
public func join(_ a: () -> Void, _ b: () -> Void) {
public func concurrently(_ a: () -> Void, _ b: () -> Void) {

Just an idea.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For context: I picked join as the typical term-of-art in this space. I'm not fully sold on concurrently yet, because join represents optional concurrency, which is important for performance at scale.

I think that it would be good to go over this API and think hard about naming & how the abstractions compose, but only once we understand the performance limitations & constraints. (Concretely, some of the (internal) abstractions are being re-written due to performance limitations in the current structure of things.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is an established term-of-art, which means we should use it in the way that has been established. You join stuff after you've forked it, in my experience, Rayon notwithstanding. And IME when you fork, you're running the same closure in both threads of execution, with a parameter passed to indicate which invocation of the closure you got, similar to what you did with parallelFor, which I think might be better called forkJoin ersump'n.

I'm not sure the concurrency is optional from the programming model P.O.V., which is what matters here. There may or may not be any actual parallelism between a and b's execution, but that would be true even if you unconditionally launched separate threads for a and b, so surfacing that distinction as though it's significant seems like a mistake to me.

Also the word “optional” tends to imply it's up to the user, but it's not; this is up to the library.

As for putting off talking about naming and abstractions of “this API” until we know it's performance, I think if we don't do both at once, we don't know what “this API” is. You don't want to design yourself into performance constraints based on assumptions about the programming model that don't actually apply.

a()
b()
}

/// Executes `a` and `b` optionally in parallel, and returns when both are complete.
///
/// Note: this implementation simply executes them serially.
public func join(_ a: () throws -> Void, _ b: () throws -> Void) throws {
try a()
try b()
}

public func parallelFor(n: Int, _ fn: VectorizedParallelForFunction) {
fn(0, n, n)
}

/// Executes `a` and `b` and returns when both are complete.
public func join(_ a: (Self) throws -> Void, _ b: (Self) throws -> Void) throws {
try a(self)
try b(self)
public func parallelFor(n: Int, _ fn: ThrowingVectorizedParallelForFunction) throws {
try fn(0, n, n)
}
}

Expand Down
Loading