Skip to content

Conversation

@bkietz
Copy link
Member

@bkietz bkietz commented Nov 16, 2020

Adds Future<T>::Then(OnSuccess, OnFailure) which registers callbacks to be executed on completion of the future and yields a future which wraps the result of those callbacks; if a callback returns:

  • void, Then() returns a Future<> which completes successully as soon as the callback runs.
  • Status, Then() returns a Future<> which completes with the returned Status as soon as the callback runs.
  • V or Result<V>, Then() returns a Future<V> which completes with whatever the callback returns.
  • Future<V>, Then() returns a Future<V> which will be marked complete when the future returned by the callback completes (and will complete with the same result).

@bkietz bkietz requested a review from pitrou November 16, 2020 17:13
@github-actions
Copy link

@bkietz bkietz force-pushed the feature/arrow-10182 branch 3 times, most recently from 94c0886 to 51b699b Compare November 16, 2020 20:57
@bkietz
Copy link
Member Author

bkietz commented Nov 16, 2020

Added a benchmark to measure the cost of creating a Future in Executor::Submit instead of just using Executor::Spawn. Locally:

---------------------------------------------------------------------------------------------
Benchmark                                                      Time           CPU Iterations
---------------------------------------------------------------------------------------------
ThreadPoolSpawn/threads:4/task_cost:1000/real_time     201939036 ns  173389187 ns          6    967.19k items/s
ThreadPoolSpawn/threads:8/task_cost:1000/real_time     294998893 ns  264699658 ns          2   662.082k items/s
ThreadPoolSpawn/threads:4/task_cost:10000/real_time     36495616 ns    2546067 ns         19   535.194k items/s
ThreadPoolSpawn/threads:8/task_cost:10000/real_time     20151885 ns    3711374 ns         30   969.251k items/s
ThreadPoolSpawn/threads:4/task_cost:100000/real_time    36728060 ns     285424 ns         19   53.2046k items/s
ThreadPoolSpawn/threads:8/task_cost:100000/real_time    23104241 ns     415593 ns         29   84.5776k items/s
ThreadPoolSubmit/threads:4/task_cost:1000/real_time     20986410 ns   19332114 ns         33   465.377k items/s
ThreadPoolSubmit/threads:8/task_cost:1000/real_time     19378929 ns   18328342 ns         36    503.98k items/s
ThreadPoolSubmit/threads:4/task_cost:10000/real_time     2320937 ns     761124 ns        303   421.183k items/s
ThreadPoolSubmit/threads:8/task_cost:10000/real_time     1727973 ns    1442820 ns        393   565.714k items/s
ThreadPoolSubmit/threads:4/task_cost:100000/real_time    2085426 ns     112413 ns        329   47.2962k items/s
ThreadPoolSubmit/threads:8/task_cost:100000/real_time    1279387 ns     104251 ns        514   77.0938k items/s

This shows there is significant overhead for small tasks and pools with fewer threads. Follow up: https://issues.apache.org/jira/browse/ARROW-10625

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Thanks a lot, really nice.

Copy link
Member

Choose a reason for hiding this comment

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

Nice. Do we want to use FnOnce in other places at some point? (perhaps open a JIRA?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think it should replace std::function in thread_pool.h at least

Copy link
Member Author

Choose a reason for hiding this comment

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

///
/// The callback should receive the result of the future (const Result<T>&)
/// For a void or statusy future this should be
/// (const Result<Future<detail::Empty>::ValueType>& result)
Copy link
Member

Choose a reason for hiding this comment

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

... which is? Ideally, the user gets something "simple", such as Status or const Status&.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. They receive const Result<T>& or const Result<detail::Empty>&

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's preferable to keep things as generic as possible, so the empty result is preferable to the plain Status

void AssertSuccessful(const Future<T>& fut) {
ASSERT_EQ(fut.state(), FutureState::SUCCESS);
ASSERT_OK(fut.status());
if (IsFutureFinished(fut.state())) {
Copy link
Member

Choose a reason for hiding this comment

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

This is changing the meaning of the tests, as now a pending future will pass this silently.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch. There should be an else block calling FAIL. I'll add that. The intent of this "if" wrapper is to prevent a pending future from deadlocking the test (which makes it difficult to debug). It absolutely should fail if the future is pending.

@bkietz
Copy link
Member Author

bkietz commented Dec 18, 2020

CI failures are unrelated. Will merge

Copy link
Member

@westonpace westonpace left a comment

Choose a reason for hiding this comment

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

LGTM

@bkietz bkietz closed this in 21241f2 Dec 18, 2020
@westonpace westonpace deleted the feature/arrow-10182 branch March 3, 2021 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants