Add rate-limiting support to batched MultiGet() #10159

hx235 · 2022-06-13T23:16:54Z

Context/Summary:
#9424 added rate-limiting support for user reads, which does not include batched MultiGet()s that call RandomAccessFileReader::MultiRead(). The reason is that it's harder (compared with RandomAccessFileReader::Read()) to implement the ideal rate-limiting where we first call RateLimiter::RequestToken() for allowed bytes to multi-read and then consume those bytes by satisfying as many requests in MultiRead() as possible. For example, it can be tricky to decide whether we want partially fulfilled requests within one MultiRead() or not.

However, due to a recent urgent user request, we decide to pursue an elementary (but a conditionally ineffective) solution where we accumulate enough rate limiter requests toward the total bytes needed by one MultiRead() before doing that MultiRead(). This is not ideal when the total bytes are huge as we will actually consume a huge bandwidth from rate-limiter causing a burst on disk. This is not what we ultimately want with rate limiter. Therefore a follow-up work is noted through TODO comments.

Test:

Modified existing unit test DBRateLimiterOnReadTest/DBRateLimiterOnReadTest.NewMultiGet
Traced the underlying system calls io_uring_enter and verified they are 10 seconds apart from each other correctly under the setting of strace -ftt -e trace=io_uring_enter ./db_bench -benchmarks=multireadrandom -db=/dev/shm/testdb2 -readonly -num=50 -threads=1 -multiread_batched=1 -batch_size=100 -duration=10 -rate_limiter_bytes_per_sec=200 -rate_limiter_refill_period_us=1000000 -rate_limit_bg_reads=1 -disable_auto_compactions=1 -rate_limit_user_ops=1 where each MultiRead() read about 2000 bytes (inspected by debugger) and the rate limiter grants 200 bytes per seconds.
Stress test:
- Verified ./db_stress (-test_cf_consistency=1/test_batches_snapshots=1) -use_multiget=1 -cache_size=1048576 -rate_limiter_bytes_per_sec=10241024 -rate_limit_bg_reads=1 -rate_limit_user_ops=1 work

ajkr

LGTM, thanks!

ajkr · 2022-06-14T05:14:52Z

file/random_access_file_reader.cc

+              assert(rate_limiter != nullptr);
+              size_t remaining_bytes = total_bytes_to_request;
+              size_t request_bytes = 0;
+              while (remaining_bytes > 0) {
+                request_bytes = std::min(
+                    static_cast<size_t>(rate_limiter->GetSingleBurstBytes()),
+                    remaining_bytes);
+                rate_limiter->Request(request_bytes, pri, stats, op_type);
+                remaining_bytes -= request_bytes;
+              }


This doesn't need to be executed via a function object, does it?

Hmmm - I think it's still thru function object under the hood. Actually I wanted to reach out for help on this and the PR here is just a WIP.

My goal is to encapsulate the helper function LoopRateLimitRequestHelper within this particular function ::MultiRead() to minimize its usage as much as possible (as you can see from our plan of deprecating BackupEngineImpl::LoopRateLimitRequestHelper).

So I am trying something like local (so that it can't be called outside of this function) static (so that we only have one copy) anonymous function. According the website title, "Lambda expressions - Constructs a closure: an unnamed function object capable of capturing variables in scope.", I think a function object is still constructed.

I am not sure if this is the best practice to achieve my goal though so let me know if you see alternatives!

I'd delete line 369-372 and line 383-386, and then fixup the undefined variables.

Okay - so basically just remove the function abstraction - I can do that too.

ajkr · 2022-06-14T05:18:45Z

file/random_access_file_reader.cc

+        // TODO: ideally we should request bandwith for each
+        // `FSReadRequest::len` seperately instead of requesting
+        // the sum and calling `LoopRateLimitRequestHelper()` afterwards


I am not sure the TODO describes a goal that is better than what's here already. One possible goal would be looping: RequestToken() followed by MultiRead() with as many requests as possible to consume the bytes granted by the rate limiter.

RequestToken() followed by MultiRead() with as many requests as possible to consume the bytes granted by the rate limiter.

Will reach out offline for more clarification.

hx235 · 2022-06-14T06:21:34Z

Hi @anand1976, @ajkr - I know crash test wrapper does not use rate-limiting yet but I want to at least cover "new MultiGet + rate-limiting" in db_stress (e.g, setting the ReadOptions::rate_limiter_priority for new MultiGet so that we can manually trigger it by ./db_stress --use_multiget=1 --rate_limit_bg_reads = 1 --rate_limit_user_ops.

For that, I want to invoke RandomAccessFileReader::MultiRead in db_stress just to make sure the setting works manually. I tried a simple ./db_stress --use_multiget=1 and it did not stop at RandomAcessFileReader::MultiRead. Is there any other command I can use?

ajkr · 2022-06-14T06:49:52Z

Hi @anand1976, @ajkr - I know crash test wrapper does not use rate-limiting yet but I want to at least cover "new MultiGet + rate-limiting" in db_stress (e.g, setting the ReadOptions::rate_limiter_priority for new MultiGet so that we can manually trigger it by ./db_stress --use_multiget=1 --rate_limit_bg_reads = 1 --rate_limit_user_ops.

For that, I want to invoke RandomAccessFileReader::MultiRead in db_stress just to make sure the setting works manually. I tried a simple ./db_stress --use_multiget=1 and it did not stop at RandomAcessFileReader::MultiRead. Is there any other command I can use?

Try this command. I built it incrementally but maybe the key piece is just the cache_size (apparently the default for db_stress is 2GB!)

$ ./db_stress -use_multiget=1 -write_buffer_size=524288 -max_bytes_for_level_base=2097152 -max_background_compactions=8 -compression_type=none -value_size_mult=33 -use_multiget=1 -writepercent=90 -readpercent=10 -prefixpercent=0 -delpercent=0 -delrangepercent=0 -iterpercent=0 -column_families=1 -clear_column_family_one_in=0 -cache_size=1048576

facebook-github-bot · 2022-06-14T08:46:01Z

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

hx235 · 2022-06-14T08:48:11Z

Try this command. I built it incrementally but maybe the key piece is just the cache_size (apparently the default for db_stress is 2GB!)

$ ./db_stress -use_multiget=1 -write_buffer_size=524288 -max_bytes_for_level_base=2097152 -max_background_compactions=8 -compression_type=none -value_size_mult=33 -use_multiget=1 -writepercent=90 -readpercent=10 -prefixpercent=0 -delpercent=0 -delrangepercent=0 -iterpercent=0 -column_families=1 -clear_column_family_one_in=0 -cache_size=1048576

Thanks - indeed a smaller cache is what I needed for db_stress! However, this still doesn't work for triggering multiget under --test_cf_consistency=1 (cf_consistency_stress.cc) or --test_batches_snapshots=1 (batched_ops_stress.cc) and the reason appears to be this while (!fp.IsSearchEnded()) { still never met (just like db_stress with a default cache).

~~I need some help with those two too :/~~ Got help.

facebook-github-bot · 2022-06-14T09:41:10Z

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

facebook-github-bot · 2022-06-14T09:41:26Z

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

anand1976 · 2022-06-14T17:48:09Z

HISTORY.md

@@ -32,6 +32,7 @@
 * Add a new column family option `blob_file_starting_level` to enable writing blob files during flushes and compactions starting from the specified LSM tree level.
 * Add support for timestamped snapshots (#9879)
 * Provide support for AbortIO in posix to cancel submitted asynchronous requests using io_uring.
+* Add support for rate-limiting new void-returning `MultiGet()` APIs


I suggest calling them batched MultiGet() APIs rather than new.

Will update all such reference.

facebook-github-bot · 2022-06-14T20:16:31Z

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

facebook-github-bot · 2022-06-14T20:16:46Z

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

hx235 · 2022-06-14T20:18:27Z

Will merge this after signals are clear

facebook-github-bot · 2022-06-15T22:34:30Z

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

facebook-github-bot · 2022-06-15T23:35:57Z

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

facebook-github-bot · 2022-06-16T18:42:07Z

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

facebook-github-bot · 2022-06-16T18:43:47Z

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

facebook-github-bot · 2022-06-17T04:45:30Z

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

facebook-github-bot · 2022-06-17T04:46:16Z

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

facebook-github-bot · 2022-06-17T06:53:11Z

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

facebook-github-bot · 2022-06-17T06:53:54Z

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

hx235 added the WIP Work in progress label Jun 13, 2022

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

hx235 force-pushed the multiread_internal_rl branch 2 times, most recently from d249c8c to 48a1611 Compare June 14, 2022 00:01

ajkr approved these changes Jun 14, 2022

View reviewed changes

hx235 force-pushed the multiread_internal_rl branch 2 times, most recently from ef8d321 to c686056 Compare June 14, 2022 06:09

anand1976 approved these changes Jun 14, 2022

View reviewed changes

hx235 changed the title ~~Add rate-limiting support to new MultiGet()~~ Add rate-limiting support to batched MultiGet() Jun 14, 2022

hx235 force-pushed the multiread_internal_rl branch from 3dbba60 to 2ca9659 Compare June 14, 2022 20:16

hx235 force-pushed the multiread_internal_rl branch from ca744ee to 6f06366 Compare June 17, 2022 04:45

hx235 added 4 commits June 16, 2022 23:52

Add rate-limiting support to new MultiGet()

d06c0bf

Feedback: remove LoopRateLimitRequestHelper abstraction

798074a

Feedback: add stress_test support

a70fa5b

Minor fix of typo and statement order

a6161a5

Feedback: refer as batched instead of new MultiGet()

d75dd3b

hx235 force-pushed the multiread_internal_rl branch from 6f06366 to d75dd3b Compare June 17, 2022 06:53

facebook-github-bot closed this in a5d773e Jun 17, 2022

gitbw95 mentioned this pull request Jun 18, 2022

Update HISTORY for 7.4.0 release freeze #10196

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rate-limiting support to batched MultiGet() #10159

Add rate-limiting support to batched MultiGet() #10159

hx235 commented Jun 13, 2022 •

edited

Loading

ajkr left a comment

ajkr Jun 14, 2022

hx235 Jun 14, 2022 •

edited

Loading

ajkr Jun 14, 2022

hx235 Jun 14, 2022

hx235 Jun 14, 2022

ajkr Jun 14, 2022

hx235 Jun 14, 2022

hx235 Jun 14, 2022

hx235 commented Jun 14, 2022

ajkr commented Jun 14, 2022

facebook-github-bot commented Jun 14, 2022

hx235 commented Jun 14, 2022 •

edited

Loading

facebook-github-bot commented Jun 14, 2022

facebook-github-bot commented Jun 14, 2022

anand1976 Jun 14, 2022

hx235 Jun 14, 2022

hx235 Jun 14, 2022

facebook-github-bot commented Jun 14, 2022

facebook-github-bot commented Jun 14, 2022

hx235 commented Jun 14, 2022

facebook-github-bot commented Jun 15, 2022

facebook-github-bot commented Jun 15, 2022

facebook-github-bot commented Jun 16, 2022

facebook-github-bot commented Jun 16, 2022

facebook-github-bot commented Jun 17, 2022

facebook-github-bot commented Jun 17, 2022

facebook-github-bot commented Jun 17, 2022

facebook-github-bot commented Jun 17, 2022

Add rate-limiting support to batched MultiGet() #10159

Add rate-limiting support to batched MultiGet() #10159

Conversation

hx235 commented Jun 13, 2022 • edited Loading

ajkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hx235 Jun 14, 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

hx235 commented Jun 14, 2022

ajkr commented Jun 14, 2022

facebook-github-bot commented Jun 14, 2022

hx235 commented Jun 14, 2022 • edited Loading

facebook-github-bot commented Jun 14, 2022

facebook-github-bot commented Jun 14, 2022

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 Jun 14, 2022

facebook-github-bot commented Jun 14, 2022

hx235 commented Jun 14, 2022

facebook-github-bot commented Jun 15, 2022

facebook-github-bot commented Jun 15, 2022

facebook-github-bot commented Jun 16, 2022

facebook-github-bot commented Jun 16, 2022

facebook-github-bot commented Jun 17, 2022

facebook-github-bot commented Jun 17, 2022

facebook-github-bot commented Jun 17, 2022

facebook-github-bot commented Jun 17, 2022

hx235 commented Jun 13, 2022 •

edited

Loading

hx235 Jun 14, 2022 •

edited

Loading

hx235 commented Jun 14, 2022 •

edited

Loading