Support reservation in thread pool #10278

littlepig2013 · 2022-06-29T22:43:30Z

Summary:

Add ReserveThreads and ReleaseThreads functions in thread pool to support reservation in for a specific thread pool. With this feature, a thread will be blocked if the number of waiting threads (noted by num_waiting_threads_) equals the number of reserved threads (noted by reserved_threads_), normally reserved_threads_ is upper bounded by num_waiting_threads_; in rare cases (e.g. SetBackgroundThreadsInternal is called when some threads are already reserved), num_waiting_threads_ can be less than reserved_threads.

Test Plan:
Add ReserveThreads unit test in env_test. Update the unit test SimpleColumnFamilyInfoTest in thread_list_test with adding ReserveThreads related assertions.

ajkr

Thanks for the PR, had a few comments.

Let's mention the Public API change in HISTORY.md so users know to update their Env implementation.

I think total_threads_limit_ is always accessed with ThreadPoolImpl mutex held, in which case we don't need to use atomic operations.

include/rocksdb/threadpool.h

env/env_test.cc

util/threadpool_imp.cc

ajkr

LGTM

ajkr · 2022-07-05T22:01:21Z

HISTORY.md

@@ -11,6 +11,7 @@
  * `rocksdb_level_metadata_t` and its and its get functions & destroy function.
  * `rocksdb_file_metadata_t` and its and get functions & destroy functions.
 * Add suggest_compact_range() and suggest_compact_range_cf() to C API.
+* Add two functions `int ReserveThreads(int threads_to_be_reserved)` and `int ReleaseThreads(threads_to_be_released)` into `Env` class. In the default implementation, both return 0. Newly added `xxxEnv` class that inherits `Env` should implement these two functions for compatibility.


should implement these two functions for compatibility

I think the main reason to implement them is to enable new features, initially subcompactions for round-robin compactions. The default implementations should already provide compatible API/behavior by not using subcompactions in round-robin.

ajkr · 2022-07-05T23:52:07Z

util/threadpool_imp.cc

+    return static_cast<int>(thread_id) + reserved_threads_ >=
+           total_threads_limit_;


This seems inconsistent with HasExcessiveThread(). Personally I'd not terminate/recreate thread-pool threads for reservations.

I see your concern. But if we do that, how do we pend incoming tasks when most threads are reserved?

Oh good point. I guess we could resolve the inconsistency either by changing the name IsExcessiveThread() to something else like IsBlockedThread(), or by adding back the reserved_threads_ into HasExcessiveThread(). Up to you.

I am not quite following why the new IsBlockedThread() implementation is correct in preventing extra threads being woken up as a result of not considering reserved threads. See (4) in my review summary #10278 (comment) for more.

hx235 · 2022-07-06T02:47:51Z

util/threadpool_imp.cc

+    if (reserved_threads_ + threads_to_be_reserved > num_waiting_) {
+      reserved_threads_in_success = num_waiting_ - reserved_threads_;
+      reserved_threads_ = num_waiting_;
+    } else {
+      reserved_threads_ += threads_to_be_reserved;
+    }


Is it equivalent to

reserved_threads_in_success = std::min(num_waiting_ - reserved_threads_, threads_to_be_reserved) reserved_threads_ += reserved_threads_in_success;

Similar simplification for ReleaseThreads()

Good catch! Will apply this change soon.

hx235 · 2022-07-06T02:50:14Z

util/threadpool_imp.cc

+ // Number of reserved threads by other tasks in bgthreads_
+ int reserved_threads_;
+ // Number of waiting threads (Maximum number of threads that can be reserved)
+ int num_waiting_;


nit: num_waiting_threads_

(I generally prefer more explicit naming for variable so that people don't have to look at comment to understand its intention)

hx235 · 2022-07-06T02:50:41Z

util/threadpool_imp.cc

@@ -110,6 +142,10 @@ struct ThreadPoolImpl::Impl {

 int total_threads_limit_;
 std::atomic_uint queue_len_;  // Queue length. Used for stats reporting
+ // Number of reserved threads by other tasks in bgthreads_
+ int reserved_threads_;


nit: num_reserved_threads_ to be consistent with below

"other tasks" sounds a bit random/unclear, although I understand it refers to tasks that reserve threads (e.g, subcompaction) as opposed to tasks do not reserve threads (e.g, flush) because I was exposed to this PR's motivation.

Is there a way to better clarify the comment so that people w/o much context of your PR understand it?

I see your point. I will revise it accordingly.

hx235 · 2022-07-06T02:58:43Z

util/threadpool_imp.cc

@@ -189,10 +229,14 @@ void ThreadPoolImpl::Impl::BGThread(size_t thread_id) {
    // Wait until there is an item that is ready to run
    std::unique_lock<std::mutex> lock(mu_);
    // Stop waiting if the thread needs to do work or needs to terminate.
+    // increase num_waiting_ once this task has finished


Should this be "increase num_waiting_ once the thread starts waiting" to be consistent with "decrease num_waiting_ once the thread is not waiting" below?

nit: Start with a capitalized letter in the comments to be consistent with others such as "Increase ..." or "Decrease ..." (below)

That was my typo. Thanks for pointing out this.

hx235 · 2022-07-06T03:04:46Z

util/threadpool_imp.h

+  // scheduled jobs. (This function will reset reserved_threads to 0,
+  // reset is only triggered when total_threads_limit changes, typically,
+  // (num > total_threads_limit) || (num < total_threads_limit && allow_reduce)


Comments not needed?

Ohh, thanks a lot for catching this! This was an old comment for my earlier implementation. I will remove this.

facebook-github-bot · 2022-07-07T07:40:05Z

@littlepig2013 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-07-07T15:49:16Z

@littlepig2013 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-07-07T15:50:51Z

@littlepig2013 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-07-07T16:32:02Z

@littlepig2013 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-07-07T17:00:06Z

@littlepig2013 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-07-07T17:20:30Z

@littlepig2013 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-07-07T18:17:35Z

@littlepig2013 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-07-07T18:20:59Z

@littlepig2013 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

hx235 · 2022-07-08T22:28:27Z

util/threadpool_imp.cc

@@ -155,6 +191,8 @@ void ThreadPoolImpl::Impl::JoinThreads(bool wait_for_jobs_to_complete) {
  // prevent threads from being recreated right after they're joined, in case
  // the user is concurrently submitting jobs.
  total_threads_limit_ = 0;
+  reserved_threads_ = 0;


No need to take action - I just want to note that it's interesting we are setting reserved_threads_ = 0 while ThreadPoolImpl does not actually have the power to terminate those reserved threads. IMO, this is the biggest drawback of this approach exchanged for the benefit of implementational simplicity.

hx235 · 2022-07-08T22:40:54Z

env/env_test.cc

@@ -1271,7 +1401,6 @@ TEST_P(EnvPosixTestWithParam, MultiRead) {
            }
          }
        });
-    ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->EnableProcessing();


unrelated change?

It's a bit weird since i did not change that.

hx235 · 2022-07-08T22:54:08Z

env/env_test.cc

+        "EnvTest::ReserveThreads:0"}});
+  // Empty the thread pool to ensure all the threads can start later
+  env_->SetBackgroundThreads(0, Env::Priority::HIGH);
+  ASSERT_EQ(env_->GetBackgroundThreads(Env::HIGH), 0);


So it seems to me that it's possible for ASSERT_EQ(env_->GetBackgroundThreads(Env::HIGH), 0); to be true while ThreadPoolImpl::BGThread::Termination:th0 has not been run?

More of a question from me than suggestion - should we put ASSERT_EQ(env_->GetBackgroundThreads(Env::HIGH), 0); after TEST_SYNC_POINT("EnvTest::ReserveThreads:0"); or remove ASSERT_EQ(env_->GetBackgroundThreads(Env::HIGH), 0); as what we need is actually the snyc point guarantee?

i think we are safe to remove it

hx235 · 2022-07-08T23:04:36Z

env/env_test.cc

+  // Set the number of threads to 3 so that Task 3 can dequeue
+  env_->SetBackgroundThreads(3, Env::Priority::HIGH);
+  // Wakup Task 1
+  tasks[1].WakeUp();
+  ASSERT_FALSE(tasks[1].TimedWaitUntilDone(kWaitMicros));
+  // Task 2, 3 running (Task 3 dequeue); 0/1 reserved threads
+  ASSERT_FALSE(tasks[3].TimedWaitUntilSleeping(kWaitMicros));
+  ASSERT_TRUE(tasks[3].IsSleeping());
+  ASSERT_EQ(0U, env_->GetThreadPoolQueueLen(Env::Priority::HIGH));
+
+  // At most 1 thread can be released
+  ASSERT_GT(2, env_->ReleaseThreads(3, Env::Priority::HIGH));
+  tasks[2].WakeUp();
+  ASSERT_FALSE(tasks[2].TimedWaitUntilDone(kWaitMicros));
+  tasks[3].WakeUp();
+  ASSERT_FALSE(tasks[3].TimedWaitUntilDone(kWaitMicros));
+  WaitThreadPoolsEmpty();
+  ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->DisableProcessing();


Are these tests scenarios that haven't been covered previously OR just clean-up? Just want to make sure we are also not making the test unnecessarily long due to duplicating testing scenarios.

Not really sure what 0/1 reserved threads means - is it "0 or 1 reserved thread"?

Line 964 - 968 is just clean-up.

yes, I will change it to make it more clear.

hx235

Thanks for your efforts to cover different scenarios in the UT along with great comments! Left a few nits!

facebook-github-bot · 2022-07-08T23:19:34Z

@littlepig2013 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-07-08T23:44:21Z

@littlepig2013 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary: Update HISTORY.md for CompactionPri::kRoundRobin. Detailed implementation can be found in [PR10107](#10107), [PR10227](#10227), [PR10250](#10250), [PR10278](#10278), [PR10316](#10316), and [PR10341](#10341) Pull Request resolved: #10421 Reviewed By: ajkr Differential Revision: D38194070 Pulled By: littlepig2013 fbshipit-source-id: 4ce153dc0bf22cd865d09c5429955023dbc90f37

facebook-github-bot added the CLA Signed label Jun 29, 2022

littlepig2013 force-pushed the support_reservation_in_thread_pool branch from a223e93 to c9ac504 Compare June 30, 2022 22:24

littlepig2013 changed the title ~~[DRAFT] Support reservation in thread pool~~ Support reservation in thread pool Jun 30, 2022

littlepig2013 force-pushed the support_reservation_in_thread_pool branch 6 times, most recently from edd08b6 to 1683b55 Compare July 3, 2022 01:08

littlepig2013 requested review from hx235 and ajkr July 3, 2022 01:46

littlepig2013 force-pushed the support_reservation_in_thread_pool branch 7 times, most recently from f9a747b to 5be7758 Compare July 5, 2022 18:04

ajkr requested changes Jul 5, 2022

View reviewed changes

littlepig2013 force-pushed the support_reservation_in_thread_pool branch 3 times, most recently from 8509ca3 to f9be4b7 Compare July 5, 2022 23:43

ajkr approved these changes Jul 5, 2022

View reviewed changes

littlepig2013 force-pushed the support_reservation_in_thread_pool branch 2 times, most recently from 4aa4236 to 937b831 Compare July 6, 2022 01:22

hx235 reviewed Jul 6, 2022

View reviewed changes

littlepig2013 force-pushed the support_reservation_in_thread_pool branch from 66dbda7 to fad97e1 Compare July 7, 2022 07:40

littlepig2013 force-pushed the support_reservation_in_thread_pool branch from fad97e1 to 90a70ad Compare July 7, 2022 15:49

littlepig2013 force-pushed the support_reservation_in_thread_pool branch from 90a70ad to f332ca1 Compare July 7, 2022 15:50

littlepig2013 force-pushed the support_reservation_in_thread_pool branch 2 times, most recently from 0861082 to e7f8bb9 Compare July 7, 2022 16:31

littlepig2013 force-pushed the support_reservation_in_thread_pool branch from e7f8bb9 to 5bce90c Compare July 7, 2022 17:00

littlepig2013 force-pushed the support_reservation_in_thread_pool branch from 5bce90c to db8370d Compare July 7, 2022 18:17

hx235 reviewed Jul 8, 2022

View reviewed changes

hx235 self-requested a review July 8, 2022 23:10

hx235 approved these changes Jul 8, 2022

View reviewed changes

Support reservation in thread pool

dcdfa5b

littlepig2013 force-pushed the support_reservation_in_thread_pool branch from db8370d to dcdfa5b Compare July 8, 2022 23:19

facebook-github-bot closed this in 9620653 Jul 9, 2022

siying mentioned this pull request Jul 15, 2022

Fix hang in MultiRead with O_DIRECT and io_uring #10368

Closed

littlepig2013 mentioned this pull request Jul 26, 2022

Mention kRoundRobin in HISTORY.md #10421

Closed

		return static_cast<int>(thread_id) + reserved_threads_ >=
		total_threads_limit_;

Support reservation in thread pool #10278

Support reservation in thread pool #10278

Conversation

littlepig2013 commented Jun 29, 2022 • edited Loading

ajkr left a comment

Choose a reason for hiding this comment

ajkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

littlepig2013 Jul 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hx235 Jul 6, 2022 • edited Loading

Choose a reason for hiding this comment

hx235 Jul 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 7, 2022

facebook-github-bot commented Jul 7, 2022

facebook-github-bot commented Jul 7, 2022

facebook-github-bot commented Jul 7, 2022

facebook-github-bot commented Jul 7, 2022

facebook-github-bot commented Jul 7, 2022

facebook-github-bot commented Jul 7, 2022

facebook-github-bot commented Jul 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

littlepig2013 Jul 8, 2022 • edited Loading

Choose a reason for hiding this comment

hx235 Jul 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hx235 left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 8, 2022

facebook-github-bot commented Jul 8, 2022

littlepig2013 commented Jun 29, 2022 •

edited

Loading

littlepig2013 Jul 5, 2022 •

edited

Loading

hx235 Jul 6, 2022 •

edited

Loading

hx235 Jul 6, 2022 •

edited

Loading

littlepig2013 Jul 8, 2022 •

edited

Loading

hx235 Jul 8, 2022 •

edited

Loading