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

remove async_trait from ExchangeSource #1463

Closed
lmatz opened this issue Mar 31, 2022 · 3 comments · Fixed by #3586
Closed

remove async_trait from ExchangeSource #1463

lmatz opened this issue Mar 31, 2022 · 3 comments · Fixed by #3586
Assignees
Labels
good first issue Good for newcomers help wanted Issues that need help from contributors

Comments

@lmatz
Copy link
Contributor

lmatz commented Mar 31, 2022

https://github.com/singularity-data/risingwave/blob/main/rust/rpc_client/src/compute_client.rs#L128-L132

/// Each ExchangeSource maps to one task, it takes the execution result from task chunk by chunk.
#[async_trait::async_trait]
pub trait ExchangeSource: Send {
    async fn take_data(&mut self) -> Result<Option<DataChunk>>;
}

take_data gets frequently called and is on the critical path. We may need to remove the overhead of async_trait.

Same as what #1132 did.

@lmatz lmatz added help wanted Issues that need help from contributors good first issue Good for newcomers labels Mar 31, 2022
@skyzh skyzh changed the title remove async_triat from ExchangeSource remove async_trait from ExchangeSource Mar 31, 2022
@KivenChen
Copy link
Contributor

Hi @lmatz! May I try to resolve this issue? The intuition behind it seems similar to this tutorial: https://www.skyzh.dev/posts/articles/2022-01-31-gat-async-trait/

@lmatz
Copy link
Contributor Author

lmatz commented Jun 28, 2022

Hi @lmatz! May I try to resolve this issue? The intuition behind it seems similar to this tutorial: https://www.skyzh.dev/posts/articles/2022-01-31-gat-async-trait/

Sure!

@KivenChen
Copy link
Contributor

A PR has been created to solve this issue. PTAL #3586

mergify bot added a commit that referenced this issue Jul 6, 2022
* fix(batch): remove async_trait from ExchangeSource(#1463)

* fix(batch): remove async_trait from ExchangeSource(#1463)

* fix(batch): [cfg(test)] scope problem

* fix(batch): added license header for exchange_source.rs

* refactor(storage): remove async_trait for MergeIteratorNext

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
nasnoisaac pushed a commit to nasnoisaac/risingwave that referenced this issue Aug 9, 2022
…velabs#3673)

* fix(batch): remove async_trait from ExchangeSource(risingwavelabs#1463)

* fix(batch): remove async_trait from ExchangeSource(risingwavelabs#1463)

* fix(batch): [cfg(test)] scope problem

* fix(batch): added license header for exchange_source.rs

* refactor(storage): remove async_trait for MergeIteratorNext

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Issues that need help from contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants