Make a static pinning decision based on the last level with data (not bottommost level) (#626) #684

udi-speedb · 2023-09-22T12:41:37Z

No description provided.

mrambacher · 2023-09-22T14:32:17Z

db/column_family.cc

+  return current_->storage_info();
+}
+
+int ColumnFamilyData::IsLastLevelWithData(int level) const {


mrambacher · 2023-09-22T14:32:33Z

db/column_family.cc

+  int last_level_with_data = vstorage->num_non_empty_levels() - 1;
+
+  auto is_last_level_with_data = false;
+  if ((level > 0) and (level == last_level_with_data)) {


"&&" rather than "and" ?

mrambacher · 2023-09-22T14:36:52Z

db/table_cache.cc

@@ -68,15 +68,17 @@ TableCache::TableCache(const ImmutableOptions& ioptions,
                       const FileOptions* file_options, Cache* const cache,
                       BlockCacheTracer* const block_cache_tracer,
                       const std::shared_ptr<IOTracer>& io_tracer,
-                       const std::string& db_session_id)
+                       const std::string& db_session_id,
+                       IsLastLevelWithDataFunc is_last_leve_with_data_func)


"is_last_level_with_data_func" (not leve_)

mrambacher · 2023-09-22T14:47:35Z

include/rocksdb/table_pinning_policy.h

+  TablePinningOptions(int _level, bool _is_last_level_with_data,
+                      size_t _file_size, size_t _max_file_size_for_l0_meta_pin)


Is there a reason not to have both "last level with data" and "bottom"? I know the current use case does not seem to care about the distinction but I am wondering if it could be important in the future (or if this use case proves to be incomplete).

Hilik's intention is clearly to decide based on the last level with data.
If we ever do find a use case for the bottom flag as well, we will add it. I really see no need to have it just in case.

udi-speedb · 2023-09-23T13:54:53Z

@mrambacher I have pushed a commit addressing the issues you have raised.
Are you done with the review?
I need this PR for #660 so, I really need this PR merges soon.
Thanks

mrambacher

Approved with minor comments/suggestions

mrambacher · 2023-09-25T17:36:12Z

db/db_test.cc

+    ASSERT_GT(total_num_pinned_, 0U);
+    --total_num_pinned_;
+
+    ASSERT_GE(usage_, pinned->size);
+    usage_ -= pinned->size;
+
+    if (pinned->is_last_level_with_data) {
+      ASSERT_GT(num_pinned_last_level_with_data_, 0U);


These should probably be EXPECT_xx rather than ASSERT_xx ?

Why shouldn't it be ASSERT_xx?

Why shouldn't it be ASSERT_xx?

In my experience, ASSERT should be when the top-level method is failing whereas EXPECT should be when the failure is a long way from the code.

I would suggest you run an experiment where you hit one of these inner ASSERT statements and observe how the test fails. I often find those deep ASSERT messages hard to trace

I see, however, the guideline I am following when choosing EXPECT_* rather than ASSERT_* is the following:
"The rule of thumb is to use EXPECT_* when you want the test to continue to reveal more errors after the assertion failure, and use ASSERT_* when continuing after failure doesn’t make sense"

(From the "GoogleTest Primer" on github, for some reason the url fails to work)

mrambacher · 2023-09-25T17:37:17Z

db/table_cache.h

@@ -49,12 +50,16 @@ class HistogramImpl;
 // cache, lookup is very fast. The row cache is obtained from
 // ioptions.row_cache
 class TableCache {
+ public:
+  using IsLastLevelWithDataFunc = std::function<int(int level)>;


The function should return a bool

udi-speedb · 2023-09-26T02:05:19Z

@mrambacher Thanks for the review.
I have updated per your last comment, squashed, and forced pushed.

Yuval-Ariel · 2023-09-26T06:37:35Z

@udi-speedb , the CI seems to have failed but the force push removed that commit from this PRs history.. please refrain from force pushing after approval or rerun the CI if you do.
https://github.com/speedb-io/speedb/actions/runs/6302889926/job/17110975186

ninja: job failed: ccache /usr/bin/clang++ -DBZIP2 -DGFLAGS=1 -DGFLAGS_IS_A_DLL=0 -DHAVE_PCLMUL -DHAVE_SSE42 -DLZ4 -DOS_LINUX -DROCKSDB_FALLOCATE_PRESENT -DROCKSDB_LIB_IO_POSIX -DROCKSDB_MALLOC_USABLE_SIZE -DROCKSDB_NO_DYNAMIC_EXTENSION -DROCKSDB_PLATFORM_POSIX -DROCKSDB_PORTABLE -DROCKSDB_RANGESYNC_PRESENT -DROCKSDB_SCHED_GETCPU_PRESENT -DSNAPPY -DZLIB -DZSTD -I../ -I../include -isystem ../third-party/gtest-1.8.1/fused-src -W -Wextra -Wall -pthread -Wsign-compare -Wshadow -Wno-unused-parameter -Wno-unused-variable -Woverloaded-virtual -Wnon-virtual-dtor -Wno-missing-field-initializers -Wno-strict-aliasing -Wno-invalid-offsetof -Werror -g -DROCKSDB_USE_RTTI -std=gnu++17 -MD -MT CMakeFiles/db_test.dir/db/db_test.cc.o -MF CMakeFiles/db_test.dir/db/db_test.cc.o.d -o CMakeFiles/db_test.dir/db/db_test.cc.o -c ../db/db_test.cc
../db/db_test.cc:7462:15: error: 'ToString' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
  std::string ToString() const {
              ^
../include/rocksdb/table_pinning_policy.h:94:23: note: overridden virtual function is here
  virtual std::string ToString() const = 0;
                      ^
1 error generated.

udi-speedb · 2023-09-26T08:36:50Z

@Yuval-Ariel

Can't refrain from forced push. PR was approved and I had to squash my commits. We need to decide what to do about forced push.
How come tests have passed before the forced push? Unless I am mistaken, the changes introduced by the forced push are not those causing the CI to fail now.

udi-speedb · 2023-09-26T08:38:25Z

@Yuval-Ariel As I have already noted. If a forced push introduces code changes (not squash / commit message changes only), the CI should automatically restart and merge should be blocked as long as it doesn't pass the CI

… bottommost level) (#626)

udi-speedb · 2023-09-26T18:34:19Z

@Yuval-Ariel - Manual run of the CI passed

Please merge

Yuval-Ariel · 2023-09-27T05:32:14Z

thats actually the main branch (fc2bee8).. i've run it on 6784aad and it passed.
in any case, shouldnt this go through performance before it can be merged?

udi-speedb · 2023-09-27T05:41:29Z

thats actually the main branch (fc2bee8).. i've run it on 6784aad and it passed. in any case, shouldnt this go through performance before it can be merged?

I see. Well, It's always good to know that our main branch passes the CI.
I will make sure I know how to run the CI on the desired branch next time :-)

As for performance, you are correct. Thanks

udi-speedb · 2023-09-27T05:42:42Z

@erez-speedb Please run performace tests on this branch. Thanks

udi-speedb · 2023-10-08T06:01:54Z

@erez-speedb - Please update on the results of the performance testing and if we may merge this pr or do something else. Thanks

… bottommost level) (#684)

udi-speedb requested review from mrambacher and Yuval-Ariel September 22, 2023 12:41

udi-speedb linked an issue Sep 22, 2023 that may be closed by this pull request

Static Pinning: Incorrect decision on bottom level #662

Closed

mrambacher reviewed Sep 22, 2023

View reviewed changes

mrambacher approved these changes Sep 25, 2023

View reviewed changes

udi-speedb force-pushed the 662-static-pinning-incorrect-decision-on-bottom-level branch from 4c3c246 to 218d050 Compare September 26, 2023 02:04

udi-speedb removed the request for review from Yuval-Ariel September 26, 2023 02:05

Make a static pinning decision based on the last level with data (not…

6784aad

… bottommost level) (#626)

udi-speedb force-pushed the 662-static-pinning-incorrect-decision-on-bottom-level branch from 218d050 to 6784aad Compare September 26, 2023 08:41

udi-speedb merged commit ab9df1a into main Oct 17, 2023
20 checks passed

Yuval-Ariel deleted the 662-static-pinning-incorrect-decision-on-bottom-level branch October 17, 2023 19:08

udi-speedb added a commit that referenced this pull request Nov 23, 2023

Make a static pinning decision based on the last level with data (not…

e7e4df7

… bottommost level) (#684)

udi-speedb added a commit that referenced this pull request Dec 6, 2023

Make a static pinning decision based on the last level with data (not…

bb3d092

… bottommost level) (#684)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make a static pinning decision based on the last level with data (not bottommost level) (#626) #684

Make a static pinning decision based on the last level with data (not bottommost level) (#626) #684

udi-speedb commented Sep 22, 2023

mrambacher Sep 22, 2023

udi-speedb Sep 22, 2023

mrambacher Sep 22, 2023

udi-speedb Sep 22, 2023

mrambacher Sep 22, 2023

udi-speedb Sep 22, 2023

mrambacher Sep 22, 2023

udi-speedb Sep 22, 2023

udi-speedb commented Sep 23, 2023 •

edited

Loading

mrambacher left a comment

mrambacher Sep 25, 2023

udi-speedb Sep 25, 2023

mrambacher Sep 25, 2023

udi-speedb Sep 26, 2023 •

edited

Loading

mrambacher Sep 25, 2023

udi-speedb Sep 25, 2023

udi-speedb commented Sep 26, 2023

Yuval-Ariel commented Sep 26, 2023

udi-speedb commented Sep 26, 2023

udi-speedb commented Sep 26, 2023

udi-speedb commented Sep 26, 2023 •

edited

Loading

Yuval-Ariel commented Sep 27, 2023

udi-speedb commented Sep 27, 2023

udi-speedb commented Sep 27, 2023

udi-speedb commented Oct 8, 2023

		TablePinningOptions(int _level, bool _is_last_level_with_data,
		size_t _file_size, size_t _max_file_size_for_l0_meta_pin)

Make a static pinning decision based on the last level with data (not bottommost level) (#626) #684

Make a static pinning decision based on the last level with data (not bottommost level) (#626) #684

Conversation

udi-speedb commented Sep 22, 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

Choose a reason for hiding this comment

udi-speedb commented Sep 23, 2023 • edited Loading

mrambacher left a 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 Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

udi-speedb commented Sep 26, 2023

Yuval-Ariel commented Sep 26, 2023

udi-speedb commented Sep 26, 2023

udi-speedb commented Sep 26, 2023

udi-speedb commented Sep 26, 2023 • edited Loading

Yuval-Ariel commented Sep 27, 2023

udi-speedb commented Sep 27, 2023

udi-speedb commented Sep 27, 2023

udi-speedb commented Oct 8, 2023

udi-speedb commented Sep 23, 2023 •

edited

Loading

udi-speedb Sep 26, 2023 •

edited

Loading

udi-speedb commented Sep 26, 2023 •

edited

Loading