-
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
Add Add Operator::initialize API to handle customized operation init #6422
Conversation
✅ Deploy Preview for meta-velox canceled.
|
velox/exec/Operator.h
Outdated
@@ -301,69 +301,75 @@ class Operator : public BaseRuntimeStatWriter { | |||
|
|||
virtual ~Operator() = default; | |||
|
|||
// Returns true if 'this' can accept input. Not used if operator is a source | |||
// operator, e.g. the first operator in the pipeline. | |||
virtual void init(); |
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.
Maybe "initialize".
CC: @aditi-pandit
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.
Looks nice. Thanks.
velox/exec/Operator.h
Outdated
@@ -301,69 +301,75 @@ class Operator : public BaseRuntimeStatWriter { | |||
|
|||
virtual ~Operator() = default; | |||
|
|||
// Returns true if 'this' can accept input. Not used if operator is a source | |||
// operator, e.g. the first operator in the pipeline. | |||
virtual void init(); |
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.
Let's add documentation to explain that this method is for doing any construction work that requires memory allocations. Let's also comment that Operator's constructor should not allocate memory.
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.
Updated. Thanks!
3fc06ac
to
f5ee073
Compare
@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
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.
Thanks. Please, update PR title to "Add Operator::initialize API ... "
@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@mbasmanova thanks for the quick review! |
@xiaoxmeng merged this pull request in 8f6be9d. |
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
…acebookincubator#6422) Summary: Add operator init API to handle heavy customized operator initialization work such as memory pool allocations. The operator init is called by driver operator init method when the driver starts execution for the first time. This is required as memory allocation can trigger memory arbitration which might grab the task lock for memory reclamation. This can cause deadlock as the driver creation is under the lock (see [PR](facebookincubator#6395) for details. Later on, we will add memory usage check after driver creation to enforce there is no memory allocations from memory pool on driver creation under the task lock. Pull Request resolved: facebookincubator#6422 Reviewed By: mbasmanova Differential Revision: D48979505 Pulled By: xiaoxmeng fbshipit-source-id: 68748ef179633c0ef6c99a5ef95fd99c2313d81d
…acebookincubator#6422) Summary: Add operator init API to handle heavy customized operator initialization work such as memory pool allocations. The operator init is called by driver operator init method when the driver starts execution for the first time. This is required as memory allocation can trigger memory arbitration which might grab the task lock for memory reclamation. This can cause deadlock as the driver creation is under the lock (see [PR](facebookincubator#6395) for details. Later on, we will add memory usage check after driver creation to enforce there is no memory allocations from memory pool on driver creation under the task lock. Pull Request resolved: facebookincubator#6422 Reviewed By: mbasmanova Differential Revision: D48979505 Pulled By: xiaoxmeng fbshipit-source-id: 68748ef179633c0ef6c99a5ef95fd99c2313d81d
…acebookincubator#6422) Summary: Add operator init API to handle heavy customized operator initialization work such as memory pool allocations. The operator init is called by driver operator init method when the driver starts execution for the first time. This is required as memory allocation can trigger memory arbitration which might grab the task lock for memory reclamation. This can cause deadlock as the driver creation is under the lock (see [PR](facebookincubator#6395) for details. Later on, we will add memory usage check after driver creation to enforce there is no memory allocations from memory pool on driver creation under the task lock. Pull Request resolved: facebookincubator#6422 Reviewed By: mbasmanova Differential Revision: D48979505 Pulled By: xiaoxmeng fbshipit-source-id: 68748ef179633c0ef6c99a5ef95fd99c2313d81d
…acebookincubator#6422) Summary: Add operator init API to handle heavy customized operator initialization work such as memory pool allocations. The operator init is called by driver operator init method when the driver starts execution for the first time. This is required as memory allocation can trigger memory arbitration which might grab the task lock for memory reclamation. This can cause deadlock as the driver creation is under the lock (see [PR](facebookincubator#6395) for details. Later on, we will add memory usage check after driver creation to enforce there is no memory allocations from memory pool on driver creation under the task lock. Pull Request resolved: facebookincubator#6422 Reviewed By: mbasmanova Differential Revision: D48979505 Pulled By: xiaoxmeng fbshipit-source-id: 68748ef179633c0ef6c99a5ef95fd99c2313d81d
Add operator init API to handle heavy customized operator initialization
work such as memory pool allocations. The operator init is called by
driver operator init method when the driver starts execution for the first
time. This is required as memory allocation can trigger memory arbitration
which might grab the task lock for memory reclamation. This can cause
deadlock as the driver creation is under the lock (see PR for details.
Later on, we will add memory usage check after driver creation to enforce
there is no memory allocations from memory pool on driver creation under
the task lock.