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

[core] Introduce IOContextProvider and its Policy #48231

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rynewang
Copy link
Contributor

GcsServer manages several IO Contexts and their threads, and hard codes which class uses which IO Context. This is not flexible in that we can't easily declare another IO Context and adapt some classes without a pervasive code change.

Introduces an IoContextProvider that manages IO Contexts based on a static, compile-time Policy, whose author can determine class-IO Context mapping. This way we can change the mappings in a central place without big chunk of changes everywhere.

@rynewang rynewang requested a review from a team as a code owner October 23, 2024 20:46
Signed-off-by: Ruiyang Wang <[email protected]>
Copy link
Contributor

@dayshah dayshah left a comment

Choose a reason for hiding this comment

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

this is great love the TMP

src/ray/common/asio/asio_util.h Outdated Show resolved Hide resolved
src/ray/common/asio/asio_util.h Show resolved Hide resolved
src/ray/gcs/gcs_server/gcs_server.h Outdated Show resolved Hide resolved
src/ray/common/asio/asio_util.h Show resolved Hide resolved
src/ray/common/asio/asio_util.h Outdated Show resolved Hide resolved
src/ray/gcs/gcs_server/gcs_server.h Outdated Show resolved Hide resolved
Signed-off-by: Ruiyang Wang <[email protected]>
@rynewang
Copy link
Contributor Author

Thanks @dayshah for the valuable suggestions! Applied most, except for one: we still need the unique_ptr because the InstrumentedIOContextWithThread has no default ctor so we can't easily init it in an array.

Signed-off-by: Ruiyang Wang <[email protected]>
@rynewang rynewang added the go add ONLY when ready to merge, run all tests label Oct 24, 2024
@rynewang rynewang assigned rynewang and unassigned rkooo567 Oct 24, 2024
@jjyao jjyao assigned MengjinYan and unassigned rynewang Oct 24, 2024
Copy link
Collaborator

@jjyao jjyao left a comment

Choose a reason for hiding this comment

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

Nice

src/ray/common/asio/asio_util.h Outdated Show resolved Hide resolved
src/ray/common/asio/asio_util.h Outdated Show resolved Hide resolved
src/ray/common/asio/asio_util.h Outdated Show resolved Hide resolved
src/ray/common/asio/asio_util.h Show resolved Hide resolved
src/ray/gcs/gcs_server/gcs_server.cc Show resolved Hide resolved
@@ -206,18 +207,12 @@ class GcsServer {

void TryGlobalGC();

IoContextProvider<GcsServerIoContextPolicy> io_context_provider_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm I think that'd be too elaborate since the only difference is this 1 line

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, let's keep as it is and revisit it if it will cause merge conflicts.

Signed-off-by: Ruiyang Wang <[email protected]>
@rynewang rynewang changed the title [core] Introduce IoContextProvider and its Policy [core] Introduce IOContextProvider and its Policy Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants