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

ARROW-10484: [C++] Make Future<> more generic #8591

Closed
wants to merge 7 commits into from

Conversation

bkietz
Copy link
Member

@bkietz bkietz commented Nov 4, 2020

Future<Status> and Future<void> have been removed in favor of Future<>, whose ValueType is an empty struct. This is a statusy future but still provides a result() member function, which simplifies generic code handling Future<T> since special cases which avoid result() need not be written. Additionally, Future<T> is now convertible to a Future<> which views only its status, without allocation of new state/storage. This will expedite observing the statuses of heterogeneous collections of futures.

Details:

  • DeferNotOk and ExecuteAndMarkFinished are no longer members of Future<>
  • Constructing finished Futures no longer locks the waiter mutex
  • Future now holds a shared_ptr<FutureImpl> directly. FutureImpl stores the Future's result in a type erased allocation.
  • FutureStorage<> and FutureStorageBase are obviated and have been removed
  • deleted Executor::SubmitAsFuture() since it isn't used and can be trivially replaced: ex->SubmitAsFuture(f) -> DeferNotOk(ex->Submit(f))

@bkietz bkietz requested a review from pitrou November 4, 2020 19:20
@github-actions
Copy link

github-actions bot commented Nov 4, 2020

@bkietz bkietz force-pushed the 10484-FuturevoidStatus-could-be branch 4 times, most recently from d6fe5f0 to 77df07a Compare November 6, 2020 16:08
cpp/src/arrow/util/future.h Outdated Show resolved Hide resolved
cpp/src/arrow/util/future.h Outdated Show resolved Hide resolved
@bkietz bkietz force-pushed the 10484-FuturevoidStatus-could-be branch from ff56a85 to 2a14538 Compare November 11, 2020 19:13
@pitrou
Copy link
Member

pitrou commented Nov 12, 2020

Can you rebase to get a clean ASAN build?

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.

+1

@bkietz bkietz force-pushed the 10484-FuturevoidStatus-could-be branch from 2a14538 to 893c6b7 Compare November 12, 2020 18:00
@bkietz bkietz closed this in 3051604 Nov 12, 2020
@bkietz bkietz deleted the 10484-FuturevoidStatus-could-be branch November 12, 2020 19:11
yordan-pavlov pushed a commit to yordan-pavlov/arrow that referenced this pull request Nov 14, 2020
`Future<Status>` and `Future<void>` have been removed in favor of `Future<>`, whose `ValueType` is an empty struct. This is a statusy future but still provides a `result()` member function, which simplifies generic code handling `Future<T>` since special cases which avoid `result()` need not be written. Additionally, `Future<T>` is now convertible to a `Future<>` which views only its status, without allocation of new state/storage. This will expedite observing the statuses of heterogeneous collections of futures.

Details:
- `DeferNotOk` and `ExecuteAndMarkFinished` are no longer members of `Future<>`
- Constructing finished Futures no longer locks the waiter mutex
- Future now holds a `shared_ptr<FutureImpl>` directly. `FutureImpl` stores the Future's result in a type erased allocation.
- `FutureStorage<>` and `FutureStorageBase` are obviated and have been removed
- deleted `Executor::SubmitAsFuture()` since it isn't used and can be trivially replaced: `ex->SubmitAsFuture(f)` -> `DeferNotOk(ex->Submit(f))`

Closes apache#8591 from bkietz/10484-FuturevoidStatus-could-be

Authored-by: Benjamin Kietzman <[email protected]>
Signed-off-by: Benjamin Kietzman <[email protected]>
GeorgeAp pushed a commit to sirensolutions/arrow that referenced this pull request Jun 7, 2021
`Future<Status>` and `Future<void>` have been removed in favor of `Future<>`, whose `ValueType` is an empty struct. This is a statusy future but still provides a `result()` member function, which simplifies generic code handling `Future<T>` since special cases which avoid `result()` need not be written. Additionally, `Future<T>` is now convertible to a `Future<>` which views only its status, without allocation of new state/storage. This will expedite observing the statuses of heterogeneous collections of futures.

Details:
- `DeferNotOk` and `ExecuteAndMarkFinished` are no longer members of `Future<>`
- Constructing finished Futures no longer locks the waiter mutex
- Future now holds a `shared_ptr<FutureImpl>` directly. `FutureImpl` stores the Future's result in a type erased allocation.
- `FutureStorage<>` and `FutureStorageBase` are obviated and have been removed
- deleted `Executor::SubmitAsFuture()` since it isn't used and can be trivially replaced: `ex->SubmitAsFuture(f)` -> `DeferNotOk(ex->Submit(f))`

Closes apache#8591 from bkietz/10484-FuturevoidStatus-could-be

Authored-by: Benjamin Kietzman <[email protected]>
Signed-off-by: Benjamin Kietzman <[email protected]>
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