-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Code cleanup to use concept serial / parallel to replace single-threaded / multi-threaded as task execution modes #10792
Conversation
✅ Deploy Preview for meta-velox canceled.
|
93ee298
to
ec7fadd
Compare
@tanjialiang Would you review this? Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zhztheplayer thanks for the refactor. Not sure if we want to rename things in test code.
velox/exec/Driver.h
Outdated
@@ -241,7 +241,7 @@ class BlockingState { | |||
} | |||
|
|||
/// Moves out the blocking future stored inside. Can be called only once. | |||
/// Used in single-threaded execution. | |||
/// Used in serial execution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
serial execution mode.
}; | ||
} // namespace | ||
|
||
/// A test fixture that runs cases within multi-threaded execution mode. | ||
/// A test fixture that runs cases within parallel execution mode. | ||
class SharedArbitrationTest : public SharedArbitrationTestBase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove SharedArbitrationTestBase? Let's do this first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will open a pr asap
velox/exec/tests/TableScanTest.cpp
Outdated
@@ -3277,7 +3277,7 @@ TEST_F(TableScanTest, remainingFilterLazyWithMultiReferences) { | |||
writeToFile(file->getPath(), {vector}); | |||
CursorParameters params; | |||
params.copyResult = false; | |||
params.singleThreaded = true; | |||
params.serial = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/serial/serialExecutionMode/
velox/exec/tests/utils/Cursor.h
Outdated
@@ -63,8 +63,8 @@ struct CursorParameters { | |||
|
|||
bool copyResult = true; | |||
|
|||
/// If true, use single threaded execution. | |||
bool singleThreaded = false; | |||
/// If true, use serial execution. Use parallel execution otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use serial execution mode.
velox/exec/tests/utils/Cursor.h
Outdated
/// If true, use single threaded execution. | ||
bool singleThreaded = false; | ||
/// If true, use serial execution. Use parallel execution otherwise. | ||
bool serial = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/serial/serialExecutionMode/
velox/exec/tests/utils/Cursor.cpp
Outdated
@@ -204,20 +204,20 @@ class TaskCursorBase : public TaskCursor { | |||
std::shared_ptr<folly::Executor> executor_; | |||
}; | |||
|
|||
class MultiThreadedTaskCursor : public TaskCursorBase { | |||
class ParallelTaskCursor : public TaskCursorBase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to rename this?
velox/exec/Task.cpp
Outdated
@@ -943,7 +947,7 @@ void Task::resume(std::shared_ptr<Task> self) { | |||
// event. The Driver gets enqueued by the promise realization. | |||
// | |||
// Do not continue the driver if no executor is supplied, | |||
// This usually happens in single-threaded execution. | |||
// This usually happens in serial execution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
execution mode.
97a8b8e
to
cc0d038
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zhztheplayer LGTM. Thanks!
@@ -34,17 +34,17 @@ TEST_F(AssertQueryBuilderTest, basic) { | |||
.assertResults(data); | |||
} | |||
|
|||
TEST_F(AssertQueryBuilderTest, singleThreaded) { | |||
TEST_F(AssertQueryBuilderTest, serial) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/serial/serialExecution/
@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@xiaoxmeng merged this pull request in ede7d0b. |
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
…ded / multi-threaded as task execution modes (facebookincubator#10792) Summary: A code cleanup for task execution mode concepts, to uniformly use `serial` / `parallel`. Remove usages of `single-threaded` / `multi-threaded`. Fixes facebookincubator#10745 Pull Request resolved: facebookincubator#10792 Reviewed By: Yuhta, bikramSingh91 Differential Revision: D61956385 Pulled By: xiaoxmeng fbshipit-source-id: c601451cc5059aaec304d9e7c34506856f51fbdf
…ded / multi-threaded as task execution modes (facebookincubator#10792) Summary: A code cleanup for task execution mode concepts, to uniformly use `serial` / `parallel`. Remove usages of `single-threaded` / `multi-threaded`. Fixes facebookincubator#10745 Pull Request resolved: facebookincubator#10792 Reviewed By: Yuhta, bikramSingh91 Differential Revision: D61956385 Pulled By: xiaoxmeng fbshipit-source-id: c601451cc5059aaec304d9e7c34506856f51fbdf
A code cleanup for task execution mode concepts, to uniformly use
serial
/parallel
. Remove usages ofsingle-threaded
/multi-threaded
.Fixes #10745