[core] Use 1 thread for all fibers for an actor scheduling queue. #37949

rynewang · 2023-07-31T20:31:25Z

Now we have 1 thread per submitter worker per actor to handle the fibers to submit the actor tasks. This design together with the fact that we don't stop these threads because of lack of means to stop boost fibers, makes the issue that we have unbounded number of threads in a actor process.

This PR makes all fibers for an actor run in a same thread. This makes the number of threads in an actor process bounded.

Also changed fiber_stopped_event to std::condition_variable and std::mutex.

Fixes #33957.
Fixes #38240.

jjyao · 2023-08-02T04:52:46Z

src/ray/core_worker/transport/concurrency_group_manager.cc

@@ -95,7 +94,6 @@ void ConcurrencyGroupManager<ExecutorType>::Stop() {
  }
 }

-template class ConcurrencyGroupManager<FiberState>;


Am I missing anything? Shouldn't we only have 1 fiber state (1 thread) per concurrency group. What do you mean by 1 thread per submitter worker?

Oh, we have actor scheduling queue per caller worker.

rynewang · 2023-08-02T04:55:39Z

All concurrency groups from a same worker uses a same thread. However different worker uses different ConcurrencyGroupManager<FiberState> which means different thread

On Wed, Aug 2, 2023 at 00:52 Jiajun Yao ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/ray/core_worker/transport/concurrency_group_manager.cc <#37949 (comment)>: > @@ -95,7 +94,6 @@ void ConcurrencyGroupManager<ExecutorType>::Stop() { } } -template class ConcurrencyGroupManager<FiberState>; Am I missing anything? Shouldn't we only have 1 fiber state (1 thread) per concurrency group. What do you mean by 1 thread per submitter worker? — Reply to this email directly, view it on GitHub <#37949 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANLX3XZOBCVVTARA7KA747TXTHMKRANCNFSM6AAAAAA26YXH5A> . You are receiving this because you authored the thread.Message ID: ***@***.***>

-- Best, Ruiyang

src/ray/core_worker/transport/actor_scheduling_queue.cc

rynewang · 2023-08-02T06:04:36Z

This requires changes to ConcurrencyGroupManager. Now it creates FiberState with no constructor arguments and hence any proxy object must reference to a global FiberState and keeps a concurrency group name and FDs. I feel this approach does not do much added value compared to not using ConcurrencyGroupManager at all.

On Wed, Aug 2, 2023 at 01:05 Jiajun Yao ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/ray/core_worker/transport/actor_scheduling_queue.cc <#37949 (comment)>: > @@ -39,17 +39,17 @@ ActorSchedulingQueue::ActorSchedulingQueue( ss << "\t" << concurrency_group.name << " : " << concurrency_group.max_concurrency; } RAY_LOG(INFO) << ss.str(); - fiber_state_manager_ = std::make_unique<ConcurrencyGroupManager<FiberState>>( I think a simple fix is instead of creating a fiber_state_manager per ActorSchedulingQueue, we should have a global one and pass in to the ActorSchedulingQueue constructor. Similar to how we do it for std::shared_ptr<ConcurrencyGroupManager<BoundedExecutor>> pool_manager, ? — Reply to this email directly, view it on GitHub <#37949 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANLX3X4E3WM3GWROP5QG24DXTHNZLANCNFSM6AAAAAA26YXH5A> . You are receiving this because you authored the thread.Message ID: ***@***.***>

-- Best, Ruiyang

rkooo567

Can we add a test in the Python layer with the repro script?

jjyao

Nice.

src/ray/core_worker/transport/direct_actor_transport.cc

jjyao · 2023-08-02T19:46:20Z

src/ray/core_worker/fiber.h

@@ -18,6 +18,8 @@
 #include <chrono>

 #include "ray/util/logging.h"
+#include "ray/util/macros.h"


Is this needed

needed for RAY_UNUSED. Not sure why it did not complain before.

python/ray/tests/test_actor_bounded_threads.py

src/ray/core_worker/test/fiber_state_test.cc

… rate limiters Signed-off-by: Ruiyang Wang <[email protected]>

Signed-off-by: Ruiyang Wang <[email protected]>

…added python tests. Signed-off-by: Ruiyang Wang <[email protected]>

Signed-off-by: Ruiyang Wang <[email protected]>

rynewang · 2023-08-07T16:37:20Z

There's a ASAN & TSAN error I believe originates from boostorg/fiber#214. Trying to add flags to solve...

Signed-off-by: Ruiyang Wang <[email protected]>

rynewang · 2023-08-08T14:54:01Z

Turns out the ASAN is not from boost issues but from our own variable lifetime management. Fixed.

Signed-off-by: Ruiyang Wang <[email protected]>

… into single-thread-for-fibers-per-actor

Signed-off-by: Ruiyang Wang <[email protected]>

src/ray/core_worker/fiber.h

jjyao · 2023-08-09T18:34:41Z

src/ray/core_worker/fiber.h

@@ -124,7 +134,7 @@ class FiberState {
              // no fibers can run after this point as we don't yield here.
              // This makes sure this thread won't accidentally
              // access being destructed core worker.
-              fiber_stopped_event_.Notify();
+              fiber_stopped_event->Notify();


Let's add a fiber_stopped_event->clear() to explicitly free the pointer.

can't do clear because lambda captures the shared ptr as const. We can mark it mutable but I guess that's overkill.

Signed-off-by: Ruiyang Wang <[email protected]>

…y-project#37949) Now we have 1 thread per submitter worker per actor to handle the fibers to submit the actor tasks. This design together with the fact that we don't stop these threads because of lack of means to stop boost fibers, makes the issue that we have unbounded number of threads in a actor process. This PR makes all fibers for an actor run in a same thread. This makes the number of threads in an actor process bounded. Also changed fiber_stopped_event to std::condition_variable and std::mutex. Signed-off-by: Ruiyang Wang <[email protected]> Signed-off-by: NripeshN <[email protected]>

…y-project#37949) Now we have 1 thread per submitter worker per actor to handle the fibers to submit the actor tasks. This design together with the fact that we don't stop these threads because of lack of means to stop boost fibers, makes the issue that we have unbounded number of threads in a actor process. This PR makes all fibers for an actor run in a same thread. This makes the number of threads in an actor process bounded. Also changed fiber_stopped_event to std::condition_variable and std::mutex. Signed-off-by: Ruiyang Wang <[email protected]> Signed-off-by: e428265 <[email protected]>

…y-project#37949) Now we have 1 thread per submitter worker per actor to handle the fibers to submit the actor tasks. This design together with the fact that we don't stop these threads because of lack of means to stop boost fibers, makes the issue that we have unbounded number of threads in a actor process. This PR makes all fibers for an actor run in a same thread. This makes the number of threads in an actor process bounded. Also changed fiber_stopped_event to std::condition_variable and std::mutex. Signed-off-by: Ruiyang Wang <[email protected]> Signed-off-by: Victor <[email protected]>

rynewang changed the title ~~Use 1 thread for all fibers for an actor scheduling queue.~~ [core] Use 1 thread for all fibers for an actor scheduling queue. Jul 31, 2023

rynewang assigned jjyao and rkooo567 Jul 31, 2023

jjyao reviewed Aug 2, 2023

View reviewed changes

src/ray/core_worker/transport/actor_scheduling_queue.cc Show resolved Hide resolved

rkooo567 reviewed Aug 2, 2023

View reviewed changes

rynewang force-pushed the single-thread-for-fibers-per-actor branch from 6ed925b to 30738e2 Compare August 2, 2023 18:24

jjyao reviewed Aug 2, 2023

View reviewed changes

rynewang added 9 commits August 7, 2023 10:44

Break FiberState free from ConcurrencyGroupManager and manage its own…

ea713c2

… rate limiters Signed-off-by: Ruiyang Wang <[email protected]>

add unit test, join in stop

d63a0b7

Signed-off-by: Ruiyang Wang <[email protected]>

renamed to FiberThread, shares it in direct_actor_transport.h level, …

62ad521

…added python tests. Signed-off-by: Ruiyang Wang <[email protected]>

revert fiber.h changes

c7f4b57

Signed-off-by: Ruiyang Wang <[email protected]>

prev commit was partial

1be914e

Signed-off-by: Ruiyang Wang <[email protected]>

fix per review

2ab1ca6

Signed-off-by: Ruiyang Wang <[email protected]>

add cocurrency group tests

fcc4410

Signed-off-by: Ruiyang Wang <[email protected]>

add back default pool

310746f

Signed-off-by: Ruiyang Wang <[email protected]>

remove nullptr check, add stop in unit test

3dcf8ad

Signed-off-by: Ruiyang Wang <[email protected]>

rynewang force-pushed the single-thread-for-fibers-per-actor branch from 16a6333 to 3dcf8ad Compare August 7, 2023 14:44

add BOOST_USE_ASAN to bazelrc

293bddf

Signed-off-by: Ruiyang Wang <[email protected]>

rynewang and others added 3 commits August 7, 2023 17:24

remove the extra fiber state test and all build changes around it

a029d13

Signed-off-by: Ruiyang Wang <[email protected]>

fix ASAN issue: extend fiber_stopped_event_ lifetime to thread end

c2bea0c

Signed-off-by: Ruiyang Wang <[email protected]>

Merge branch 'master' into single-thread-for-fibers-per-actor

f7081f8

rynewang added 2 commits August 8, 2023 11:18

add back new fiber state test

be43ad6

Signed-off-by: Ruiyang Wang <[email protected]>

Merge remote-tracking branch 'ryw/single-thread-for-fibers-per-actor'…

96802b5

… into single-thread-for-fibers-per-actor

rynewang mentioned this pull request Aug 9, 2023

[core][TSAN] fiber.h can't properly synchronize #38240

Closed

use std cond var to notify other thread

88c30d0

Signed-off-by: Ruiyang Wang <[email protected]>

rynewang force-pushed the single-thread-for-fibers-per-actor branch from 18bd679 to 88c30d0 Compare August 9, 2023 00:49

Merge branch 'master' into single-thread-for-fibers-per-actor

115b2f6

jjyao approved these changes Aug 9, 2023

View reviewed changes

rynewang added 2 commits August 9, 2023 16:40

change wording

5a6efd0

Signed-off-by: Ruiyang Wang <[email protected]>

delete bad comment

06dbf2d

Signed-off-by: Ruiyang Wang <[email protected]>

jjyao merged commit 0d6126f into ray-project:master Aug 9, 2023

rynewang deleted the single-thread-for-fibers-per-actor branch August 9, 2023 20:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Use 1 thread for all fibers for an actor scheduling queue. #37949

[core] Use 1 thread for all fibers for an actor scheduling queue. #37949

rynewang commented Jul 31, 2023 •

edited

Loading

jjyao Aug 2, 2023

jjyao Aug 2, 2023

rynewang commented Aug 2, 2023 via email

rynewang commented Aug 2, 2023 via email

rkooo567 left a comment

jjyao left a comment

jjyao Aug 2, 2023

rynewang Aug 2, 2023

rynewang commented Aug 7, 2023

rynewang commented Aug 8, 2023

jjyao Aug 9, 2023

rynewang Aug 9, 2023

[core] Use 1 thread for all fibers for an actor scheduling queue. #37949

[core] Use 1 thread for all fibers for an actor scheduling queue. #37949

Conversation

rynewang commented Jul 31, 2023 • edited Loading

jjyao Aug 2, 2023

Choose a reason for hiding this comment

jjyao Aug 2, 2023

Choose a reason for hiding this comment

rynewang commented Aug 2, 2023 via email

rynewang commented Aug 2, 2023 via email

rkooo567 left a comment

Choose a reason for hiding this comment

jjyao left a comment

Choose a reason for hiding this comment

jjyao Aug 2, 2023

Choose a reason for hiding this comment

rynewang Aug 2, 2023

Choose a reason for hiding this comment

rynewang commented Aug 7, 2023

rynewang commented Aug 8, 2023

jjyao Aug 9, 2023

Choose a reason for hiding this comment

rynewang Aug 9, 2023

Choose a reason for hiding this comment

rynewang commented Jul 31, 2023 •

edited

Loading