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

feat(services/monoiofs): monoio wrapper #4885

Merged
merged 5 commits into from
Jul 18, 2024
Merged

Conversation

NKID00
Copy link
Contributor

@NKID00 NKID00 commented Jul 12, 2024

Part of #4552.

Since monoio runtime can't cooperate with tokio runtime on the same thread, this PR introduces a wrapper in monoiofs that wraps monoio runtime in a thread pool and provides a method that may be called in context of other runtimes to dispatch futures to the thread pool and execute it in context of monoio.

This is similar to the implementation of compfs, however since monoio doesn't provide a crate like compio-dispatcher to do these, I've implemented a minimal usable dispatcher here. This dispatcher implementation sends futures to threads at random (same as compio-dispatcher) without scheduling or load balancing, but it should serve for implementing rest of the service on top of it and we can improve its performance later if necessary.

@Xuanwo
Copy link
Member

Xuanwo commented Jul 12, 2024

Since monoio runtime can't cooperate with tokio runtime on the same thread, this PR introduces a wrapper in monoiofs that wraps monoio runtime in a thread pool and provides a method that may be called in context of other runtimes to dispatch futures to the thread pool and execute it in context of monoio.

This is similar to the implementation of compfs, however since monoio doesn't provide a crate like compio-dispatcher to do these, I've implemented a minimal usable dispatcher here. This dispatcher implementation sends futures to threads at random (same as compio-dispatcher) without scheduling or load balancing, but it should serve for implementing rest of the service on top of it and we can improve its performance later if necessary.

That sounds great. I recall @ihciah mentioning that they've done some work internally to make monoio compatible with the tokio runtime. Do you think this feature should be integrated into the upstream monoio?

@NKID00
Copy link
Contributor Author

NKID00 commented Jul 15, 2024

That sounds great. I recall ihciah mentioning that they've done some work internally to make monoio compatible with the tokio runtime.

Yes, there is already a compatibility layer crate monoio-compat. But AFAIK, this crate focuses only on api-level compatibility with tokio. It mainly facilitates porting existing tokio-based code (especially network related) to monoio and still requires user to run the code in context of monoio. However the goal of monoiofs is to operate even in context of other runtime such as tokio, and we don't need to port existing code either.

Do you think this feature should be integrated into the upstream monoio?

My code here only focuses on our use case inside monoiofs. There are a few issue comments in monoio requesting similar feature but most of them concern other scenarios. Maybe we can later generalize it and contribute that to monoio.

core/src/services/monoiofs/backend.rs Outdated Show resolved Hide resolved
core/src/services/monoiofs/core.rs Show resolved Hide resolved
@NKID00 NKID00 requested a review from Xuanwo July 16, 2024 04:41
@Xuanwo
Copy link
Member

Xuanwo commented Jul 18, 2024

Hi, @NKID00, thanks for your update. This PR is mostly ready to merge, can you help resolving the conflicts?

@NKID00
Copy link
Contributor Author

NKID00 commented Jul 18, 2024

Hi, NKID00, thanks for your update. This PR is mostly ready to merge, can you help resolving the conflicts?

Sure.

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks, let's move!

@Xuanwo Xuanwo merged commit 7985cfd into apache:main Jul 18, 2024
212 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants