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

Change: replace async_trait with RPITIT #996

Closed
wants to merge 0 commits into from

Conversation

hadronzoo
Copy link
Contributor

@hadronzoo hadronzoo commented Jan 28, 2024

Completes #959 by removing async_trait and updating the add_async_trait proc macro to add Send bounds when the singlethreaded feature is not enabled. This also removes the requirement for annotating trait implementations with a proc macro.

For example, the original RaftRuntime trait:

#[add_async_trait]
pub(crate) trait RaftRuntime<C: RaftTypeConfig> {
    async fn run_command<'e>(&mut self, cmd: Command<C>) -> Result<Option<Command<C>>, StorageError<C::NodeId>>;
}

is transformed into the following when the singlethreaded feature is not enabled:

pub(crate) trait RaftRuntime<C: RaftTypeConfig>: Send {
    fn run_command<'e>(&mut self, cmd: Command<C>) -> impl std::future::Future<Output=Result<Option<Command<C>>, StorageError<C::NodeId>>> + Send;
}

This PR is covered by existing unit tests.


This change is Reviewable

@hadronzoo hadronzoo changed the title Perf: replace async_trait with RPITIT Change: replace async_trait with RPITIT Jan 28, 2024
@hadronzoo hadronzoo force-pushed the native-async-traits branch 3 times, most recently from 089ac27 to 498efe4 Compare January 28, 2024 09:05
@schreter schreter requested a review from drmingdrmer January 28, 2024 11:31
Copy link
Collaborator

@schreter schreter left a comment

Choose a reason for hiding this comment

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

Nice!

From my side, it looks good.

+reviewer:@drmingdrmer

It would be also nice to measure in a local microbenchmark (so the network is excluded) the performance before and after the change - it namely significantly reduces the number of (expensive) allocations for futures and enables much better inlining across async functions.

Reviewed 34 of 34 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @drmingdrmer)

Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

It's pretty nice! It's a very clean approach! Thank you!

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @hadronzoo)

@drmingdrmer
Copy link
Member

Nice!

From my side, it looks good.

+reviewer:@drmingdrmer

It would be also nice to measure in a local microbenchmark (so the network is excluded) the performance before and after the change - it namely significantly reduces the number of (expensive) allocations for futures and enables much better inlining across async functions.

Reviewed 34 of 34 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @drmingdrmer)

make bench_cluster_of_3 shows there is a performance improvement of 6%. :D
1,016 op/ms to 1,077 op/ms

@drmingdrmer
Copy link
Member

drmingdrmer commented Jan 28, 2024

@hadronzoo
I manually merged your PR into main branch with a fix of an unused crate async_trait in crate rocksstore-v2.
And it looks like github just closed this PR because of the change to main :(

image

@hadronzoo
Copy link
Contributor Author

@drmingdrmer thank you for accepting this change!

@hadronzoo hadronzoo deleted the native-async-traits branch January 28, 2024 20:30
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.

3 participants