Introduce a TablePinningPolicy to control how and when memory is pinned #459

mrambacher · 2023-04-02T05:42:12Z

This PR adds a TablePinningPolicy to the BlockBasedTableOptions. This new class is Customizable and can be created from strings. The class has methods to limit the amount of pin data in the system.

The basic approach is that the unit probes (MayPin) if it believes data is available at the time. If so, the code does it normal checks and then attempts to pin the new data (PinData). Once the code is done with the data, UnPinData puts the memory back into the available pool

udi-speedb · 2023-05-02T04:42:16Z

db/db_block_cache_test.cc

+
+    options.ignore_unsupported_options = false;
+    const auto& id = GetParam();
+    EXPECT_OK(


Why EXPECT_OK and not ASSERT_OK?

udi-speedb · 2023-05-02T04:49:59Z

db/db_block_cache_test.cc

+  ASSERT_EQ(pinning_policy_->GetUsage(), 0);
+}
+
+static const std::vector<std::string>& GetPinningPolicies() {


Really minor, but unnamed namespaces should be the way to go instead of static functions.

udi-speedb · 2023-05-02T05:00:28Z

table/block_based/table_pinning_policy.h

+struct ConfigOptions;
+// Struct that contains information about the table being evaluated for pinning
+struct TablePinningOptions {
+  TablePinningOptions() {}


Did you choos the {} and not = default on purpose?

udi-speedb · 2023-05-02T05:05:07Z

db/db_block_cache_test.cc

+    }
+    ASSERT_EQ(pinning_policy_->GetUsage(), used);
+  }
+  printf("MJR: Pinned=%d\n", (int)used);


Is printing really necessary? I would avoid printing to the console during unit tests as it just bothers users running unit tests who just want to see the tests pass.
The unit test should use assertions to verify it is passing.

udi-speedb · 2023-05-02T05:06:59Z

db/db_block_cache_test.cc

+    const auto& id = GetParam();
+    EXPECT_OK(
+        TablePinningPolicy::CreateFromString(options, id, &pinning_policy_));
+    printf("MJR: Loaded pinning policy (%s)\n", id.c_str());


Please remove the printing (which, btw, may be done using std::cout in unit tests). Please also see my comment below regarding the printf there.

plugin/speedb/pinning_policy/scoped_pinning_policy.cc

plugin/speedb/pinning_policy/scoped_pinning_policy_test.cc

udi-speedb · 2023-05-03T14:12:12Z

@mrambacher - First batch of comments. Haven't finished the review.

udi-speedb · 2023-05-04T14:19:15Z

@mrambacher - In case it wasn't clear. I am waiting for you to address the comments I have given before continuing my review.

udi-speedb · 2023-05-14T02:41:28Z

plugin/speedb/pinning_policy/scoped_pinning_policy.cc

@@ -34,13 +34,32 @@ static std::unordered_map<std::string, OptionTypeInfo>
          OptionVerificationType::kNormal, OptionTypeFlags::kNone}},
 };

-ScopedPinningPolicy::ScopedPinningPolicy() {
+static const uint8_t kNumTypes = 7;
+static const int kNumLevels = 7;


Is that just for testing the concept?

udi-speedb · 2023-05-14T02:42:59Z

plugin/speedb/pinning_policy/scoped_pinning_policy.h

+
+  std::atomic<size_t> usage_;
+  std::vector<std::atomic_size_t> usage_by_level_;
+  std::vector<std::atomic_size_t> usage_by_type_;


Why not initialize the vectors to 0 here and avoid the necessity in the ctor?

udi-speedb · 2023-05-14T02:48:34Z

plugin/speedb/pinning_policy/scoped_pinning_policy.cc

+  }
+  for (uint8_t t = 0; t < kNumTypes; t++) {
+    usage_by_type_[t].store(0);
+  }


Couldn't you use std::fill?
But a better options IMO is to init all of the fields in the header as I suggest
And you could delegate to the non default ctor by calling here ScopedPinningPolicy(ScopedPinningOptions()) and avoid the code duplication in the ctor-s.

udi-speedb · 2023-05-14T02:48:53Z

plugin/speedb/pinning_policy/scoped_pinning_policy.cc

+  }
+  for (uint8_t t = 0; t < kNumTypes; t++) {
+    usage_by_type_[t].store(0);
+  }


Please see my comments above for the default ctor.

udi-speedb · 2023-05-14T02:50:46Z

plugin/speedb/pinning_policy/scoped_pinning_policy.cc

+    usage_by_type_[type] += size;
+  } else {
+    usage_by_level_[level] += size;
+    usage_by_type_[type] += size;


Shouldn't you decrease the usage by level / type when pinned == false?

udi-speedb · 2023-05-14T02:51:11Z

plugin/speedb/pinning_policy/scoped_pinning_policy.cc

-  } else if (pinned) {
-    usage_by_type_[type] = size;
+  if (level >= kNumLevels) level = kNumLevels - 1;
+  if (type >= kNumTypes) type = kNumTypes - 1;


Only for testing the concept or is that how it's going to be in production code?

udi-speedb · 2023-05-14T03:03:38Z

plugin/speedb/pinning_policy/scoped_pinning_policy.cc

@@ -49,78 +68,52 @@ std::string ScopedPinningPolicy::GetId() const {

 bool ScopedPinningPolicy::MayPin(const TablePinningOptions& tpo, uint8_t type,
                                 size_t size) const {
-  auto limit = GetUsage();
-  return CheckPin(tpo, type, size, limit);
+  return CheckPin(tpo, type, size, usage_);


What is MayPin()? used for?
And, it should be clear to the users that they MayPin() may return true but actual pinning may still fail.

erez-speedb · 2023-06-11T13:07:54Z

build db_bench failed
CC plugin/speedb/speedb_registry.o
CC plugin/speedb/memtable/hash_spd_rep.o
CC plugin/speedb/paired_filter/speedb_paired_bloom.o
CC plugin/speedb/paired_filter/speedb_paired_bloom_internal.o
CC plugin/speedb/pinning_policy/scoped_pinning_policy.o
CC utilities/transactions/lock/range/range_tree/lib/locktree/concurrent_tree.o
CC utilities/transactions/lock/range/range_tree/lib/locktree/keyrange.o
CC utilities/transactions/lock/range/range_tree/lib/locktree/lock_request.o
CC utilities/transactions/lock/range/range_tree/lib/locktree/locktree.o
CC utilities/transactions/lock/range/range_tree/lib/locktree/manager.o
CC utilities/transactions/lock/range/range_tree/lib/locktree/range_buffer.o
CC utilities/transactions/lock/range/range_tree/lib/locktree/treenode.o
CC utilities/transactions/lock/range/range_tree/lib/locktree/txnid_set.o
CC utilities/transactions/lock/range/range_tree/lib/locktree/wfg.o
plugin/speedb/pinning_policy/scoped_pinning_policy.cc: In member function ‘virtual bool rocksdb::ScopedPinningPolicy::CheckPin(const rocksdb::TablePinningOptions&, uint8_t, size_t, size_t) const’:
plugin/speedb/pinning_policy/scoped_pinning_policy.cc:56:48: error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits]
56 | if (tpo.is_bottom && options_.bottom_percent >= 0) {
| ~~~~~~~~~~~~~~~~~~~~~~~~^~~~
plugin/speedb/pinning_policy/scoped_pinning_policy.cc:60:52: error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits]
60 | } else if (tpo.level > 0 && options_.mid_percent >= 0) {
| ~~~~~~~~~~~~~~~~~~~~~^~~~
CC utilities/transactions/lock/range/range_tree/lib/standalone_port.o
CC utilities/transactions/lock/range/range_tree/lib/util/dbt.o
CC utilities/transactions/lock/range/range_tree/lib/util/memarena.o
CC utilities/transactions/lock/range/range_tree/range_tree_lock_manager.o
CC utilities/transactions/lock/range/range_tree/range_tree_lock_tracker.o
cc1plus: all warnings being treated as errors
make: *** [Makefile:2418: plugin/speedb/pinning_policy/scoped_pinning_policy.o] Error 1

comparison of unsigned expression >= 0 is always true [-Werror=type-limits]

Yuval-Ariel · 2023-06-21T07:07:01Z

blocked until #573 is fixed @maxb-io FYI

empty

Yuval-Ariel · 2023-07-03T13:00:26Z

@mrambacher plz also add to the HISTORY.md file

Yuval-Ariel · 2023-07-04T07:29:57Z

https://github.com/speedb-io/speedb/actions/runs/5444728254/jobs/9904106132#step:6:571

==15027==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x606000010398 at pc 0x7f21e95a2c2e bp 0x7f21dcdb9f20 sp 0x7f21dcdb9f10
WRITE of size 8 at 0x606000010398 thread T6
#0 0x7f21e95a2c2d in std::__atomic_base::operator+=(unsigned long) /usr/include/c++/7/bits/atomic_base.h:312
#1 0x7f21e95a2c2d in rocksdb::RecordingPinningPolicy::RecordPinned(int, unsigned char, unsigned long, bool) table/block_based/table_pinning_policy.cc:145
#2 0x7f21e95a2e92 in rocksdb::RecordingPinningPolicy::PinData(rocksdb::TablePinningOptions const&, unsigned char, unsigned long, std::unique_ptr<rocksdb::PinnedEntry, std::default_deleterocksdb::PinnedEntry >) table/block_based/table_pinning_policy.cc:126
#3 0x7f21e959030e in rocksdb::PartitionIndexReader::Create(rocksdb::BlockBasedTable const, rocksdb::ReadOptions const&, rocksdb::TablePinningOptions const&, rocksdb::FilePrefetchBuffer*, bool, bool, bool, rocksdb::BlockCacheLookupContext*,

Not sure how it disappeared as part of the rebase...

db_stress_tool/db_stress_gflags.cc

Yuval-Ariel

remove foo file.

Yuval-Ariel · 2023-07-24T06:02:38Z

failed unit tests:
FAILED TESTS (1/11819):
752 ms: /__w/speedb/speedb/options_settable_test OptionsSettableTest.BlockBasedTableOptionsAllFieldsSettable

…memory is pinned (#459)

mrambacher requested review from hilikspdb and udi-speedb April 2, 2023 05:42

Guyme linked an issue Apr 13, 2023 that may be closed by this pull request

Clean data phase 1: Pinning metadata to the cache #388

Closed

Guyme assigned mrambacher Apr 13, 2023

udi-speedb requested changes May 3, 2023

View reviewed changes

Yuval-Ariel force-pushed the main branch from 5817370 to 2ffee5b Compare May 11, 2023 05:50

udi-speedb self-requested a review May 14, 2023 02:38

udi-speedb requested changes May 14, 2023

View reviewed changes

udi-speedb approved these changes May 31, 2023

View reviewed changes

udi-speedb self-requested a review May 31, 2023 01:37

mrambacher added 2 commits May 30, 2023 21:58

Add TablePinningPolicy

2328aac

Change UnPinData signature

f0b89b4

mrambacher force-pushed the PinningPolicy branch from 54f92b8 to f0b89b4 Compare May 31, 2023 09:56

Address PR Comments

d0ca222

udi-speedb approved these changes Jun 6, 2023

View reviewed changes

mrambacher added 2 commits June 12, 2023 16:30

Merge remote-tracking branch 'upstream/main' into PinningPolicy

28a230e

Fix build issue

616d26e

comparison of unsigned expression >= 0 is always true [-Werror=type-limits]

udi-speedb approved these changes Jun 13, 2023

View reviewed changes

Fix sizing

cb21363

udi-speedb self-requested a review June 14, 2023 06:52

Dummy

3fa9248

Move TablePinningPolicy into public API

dba7026

empty

udi-speedb approved these changes Jun 25, 2023

View reviewed changes

Address Build Issue

5ec4cae

Guyme requested review from erez-speedb and removed request for erez-speedb June 29, 2023 12:12

Guyme assigned erez-speedb Jun 29, 2023

Merge remote-tracking branch 'upstream/main' into PinningPolicy

b256613

Yuval-Ariel self-requested a review July 3, 2023 12:56

Yuval-Ariel approved these changes Jul 3, 2023

View reviewed changes

Update HISTORY

1676960

mrambacher added 2 commits July 10, 2023 09:00

Add PinningPolicy to db_bench

823bb77

Not sure how it disappeared as part of the rebase...

Add PinningPolicy to db_stress

3ce8b78

Yuval-Ariel reviewed Jul 16, 2023

View reviewed changes

db_stress_tool/db_stress_gflags.cc Show resolved Hide resolved

mrambacher added 2 commits July 19, 2023 09:50

Fix ASAN error; add Scoped Pinning Policy to crash test

00346ea

Merge remote-tracking branch 'upstream/main' into PinningPolicy

ca648ad

Yuval-Ariel requested changes Jul 23, 2023

View reviewed changes

Remove accidental file

3be639d

Yuval-Ariel approved these changes Jul 23, 2023

View reviewed changes

Fix failing unit test

acdc0d4

Yuval-Ariel approved these changes Jul 24, 2023

View reviewed changes

Test

858f309

Yuval-Ariel merged commit 6e05428 into speedb-io:main Aug 2, 2023
1 check passed

RoyBenMoshe mentioned this pull request Aug 6, 2023

Add TablePinningPolicy to EnableSpeedbFeaturesCF function #628

Closed

udi-speedb pushed a commit that referenced this pull request Nov 22, 2023

Clean Memory: Introduce a TablePinningPolicy to control how and when …

d8f314e

…memory is pinned (#459)

udi-speedb pushed a commit that referenced this pull request Dec 5, 2023

Clean Memory: Introduce a TablePinningPolicy to control how and when …

2323270

…memory is pinned (#459)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a TablePinningPolicy to control how and when memory is pinned #459

Introduce a TablePinningPolicy to control how and when memory is pinned #459

mrambacher commented Apr 2, 2023

udi-speedb May 2, 2023

udi-speedb May 2, 2023

udi-speedb May 2, 2023

udi-speedb May 2, 2023

udi-speedb May 2, 2023

udi-speedb commented May 3, 2023

udi-speedb commented May 4, 2023

udi-speedb May 14, 2023

udi-speedb May 14, 2023

udi-speedb May 14, 2023

udi-speedb May 14, 2023

udi-speedb May 14, 2023

udi-speedb May 14, 2023

udi-speedb May 14, 2023

erez-speedb commented Jun 11, 2023 •

edited

Loading

Yuval-Ariel commented Jun 21, 2023

Yuval-Ariel commented Jul 3, 2023

Yuval-Ariel commented Jul 4, 2023

Yuval-Ariel left a comment

Yuval-Ariel commented Jul 24, 2023

Introduce a TablePinningPolicy to control how and when memory is pinned #459

Introduce a TablePinningPolicy to control how and when memory is pinned #459

Conversation

mrambacher commented Apr 2, 2023

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

udi-speedb commented May 3, 2023

udi-speedb commented May 4, 2023

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

erez-speedb commented Jun 11, 2023 • edited Loading

Yuval-Ariel commented Jun 21, 2023

Yuval-Ariel commented Jul 3, 2023

Yuval-Ariel commented Jul 4, 2023

Yuval-Ariel left a comment

Choose a reason for hiding this comment

Yuval-Ariel commented Jul 24, 2023

erez-speedb commented Jun 11, 2023 •

edited

Loading