Track each SST's timestamp information as user properties #9093

sunlike-Lipo · 2021-10-29T11:56:09Z

Track each SST's timestamp information as user properties #8959

Rockdb has supported user-defined timestamp feature. Application can specify a timestamp
when writing each k-v pair. When data flush from memory to disk file called SST files.
Each SST files consist of multiple data blocks and several metadata blocks. Among the metadata
blocks, there is one called Properties block that tracks some pre-defined properties of this SST file.

This PR is for collecting the properties of min and max timestamps of all keys in the file. With those
properties the SST file is more convenient to tell whether the keys in the SST have timestamps or not.

The changes involved are as follows:

Add a class TimestampTablePropertiesCollector to collect min/max timestamp when add keys to table,
The way TimestampTablePropertiesCollector use to compare timestamp of key should defined by
user by implementing the Comparator::CompareTimestamp function in the user defined comparator.
Add corresponding unit tests.

wolfkdy · 2021-11-04T01:42:32Z

@riversand963 I've reviewed this mr and my colleague post it.
The original mr is splitted into two mrs.
What you commented in the original mr are all fixed.
Shall you take a look when free, thanks!

riversand963

Thanks @sunlike-Lipo for the PR. I think this PR needs more testing once #9092 gets merged. cc @wolfkdy

db/table_properties_collector.h

riversand963 · 2021-11-04T03:32:58Z

db/table_properties_collector.h

+  }
+
+  Status Finish(UserCollectedProperties* properties) override {
+    properties->insert({"min_timestamp", min_timestamp});


Can we check the size of min/max timestamps are both equal to cmp_->timestamp_size()?

Fine. Already fixed it.

db/table_properties_collector.h

table/block_based/block_based_table_builder.cc

db/table_properties_collector.h

wolfkdy · 2021-11-04T04:26:47Z

@riversand963

Thanks @sunlike-Lipo for the review. I think this PR needs more testing once #9092 gets merged. cc @wolfkdy

Thanks, we plan to follow other unittests for TablePropertiesCollector's subclass and add unittests for TimestampTablePropertiesCollector.

)

sunlike-Lipo · 2021-11-11T02:05:50Z

@riversand963
I fixed review comments and add a unitTest. Could you please help me to review what else needs to be modified if that is convenient.

riversand963 · 2021-11-12T18:12:33Z

@sunlike-Lipo Will take a look.
One question: do you by any chance know the overhead introduced by tracking min/max timestamps for each file, since the comparison will be done for each key? It would be nice to include some benchmarking results here. We can assume timestamp size of 8 bytes.

riversand963

A few more comments.

db/table_properties_collector.h

db/dbformat.h

db/table_properties_collector.h

facebook-github-bot · 2021-11-13T00:10:58Z

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

facebook-github-bot · 2021-11-15T01:37:49Z

@sunlike-Lipo has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-11-15T07:03:30Z

@sunlike-Lipo has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-11-15T12:33:14Z

@sunlike-Lipo has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-11-15T12:57:39Z

@sunlike-Lipo has updated the pull request. You must reimport the pull request before landing.

sunlike-Lipo · 2021-11-15T13:02:05Z

@sunlike-Lipo Will take a look. One question: do you by any chance know the overhead introduced by tracking min/max timestamps for each file, since the comparison will be done for each key? It would be nice to include some benchmarking results here. We can assume timestamp size of 8 bytes.

Sure. I'll do the test and post the benchmarking results as soon as possible.

facebook-github-bot · 2021-11-15T13:11:22Z

@sunlike-Lipo has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-11-15T13:33:27Z

@sunlike-Lipo has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-11-15T14:27:21Z

@sunlike-Lipo has updated the pull request. You must reimport the pull request before landing.

sunlike-Lipo · 2021-11-16T11:39:45Z

@sunlike-Lipo Will take a look. One question: do you by any chance know the overhead introduced by tracking min/max timestamps for each file, since the comparison will be done for each key? It would be nice to include some benchmarking results here. We can assume timestamp size of 8 bytes.

@riversand963
I wrote a unittest(sunlike-Lipo@164ea93) for the benchmark,
In general, the test runs for 10 times and collects the total time used for sstfiles' Flush.
I use the MockEnv to eliminate disk-IO's effect.

The variables are value size and collect timestamp or not. The comparator used is ComparatorWithU64Ts.
the test results are list below

	value_size=10	value_size=100	value_size=1000
collect-ts	4763ms	1544ms	659ms
not-collect-ts	4399ms	1489ms	644ms
downgrade	8.3%	3.7%	2.3%

facebook-github-bot · 2021-11-16T11:40:29Z

@sunlike-Lipo has updated the pull request. You must reimport the pull request before landing.

riversand963 · 2021-11-16T22:58:15Z

Thanks @sunlike-Lipo for addressing the comments. Will take another look soon.

I wrote a unittest

I wonder if we can move the code to a standalone source file (similar to examples/) and compile with rocksdb library built with DEBUG_LEVEL=0 and see the performance difference in opt mode too. You can also use db_bench. In unit test, I think the DEBUG_LEVEl is at least 1, thus may already have some overhead, making the performance overhead of ts comparison less obvious.

sunlike-Lipo · 2021-11-17T13:40:52Z

Thanks @sunlike-Lipo for addressing the comments. Will take another look soon.

I wrote a unittest

I wonder if we can move the code to a standalone source file (similar to examples/) and compile with rocksdb library built with DEBUG_LEVEL=0 and see the performance difference in opt mode too. You can also use db_bench. In unit test, I think the DEBUG_LEVEl is at least 1, thus may already have some overhead, making the performance overhead of ts comparison less obvious.

I fixed it to a stand alone test (sunlike-Lipo@50a1152) for the benchmark.
The testing logical is not changed:
In general, the test runs for 10 times and collects the total time used for sstfiles' Flush.
I use the MemEnv to eliminate disk-IO's effect.

The variables are value size and collect timestamp or not,. The comparator still is ComparatorWithU64Ts.
the test results are list below

	value_size=10	value_size=100	value_size=1000
collect-ts	6188ms	2206ms	255ms
not-collect-ts	5806ms	2164ms	249ms
downgrade	6.5%	1.9%	2.4%

I will also test the db_bench, the result will be soon.

sunlike-Lipo · 2021-11-18T03:15:32Z

Thanks @sunlike-Lipo for addressing the comments. Will take another look soon.

I wrote a unittest

I wonder if we can move the code to a standalone source file (similar to examples/) and compile with rocksdb library built with DEBUG_LEVEL=0 and see the performance difference in opt mode too. You can also use db_bench. In unit test, I think the DEBUG_LEVEl is at least 1, thus may already have some overhead, making the performance overhead of ts comparison less obvious.

@riversand963
The db_bench test result is here:
command:

$ TEST_TMPDIR=/tmp/rocksdb ./db_bench -benchmarks=filluniquerandom -level0_slowdown_writes_trigger 102400 -level0_stop_writes_trigger 102400  -disable_auto_compactions true -user_timestamp_size 8 -compression_type=none  -num=20000000

With ts collector : 22.8MB/s, 22.5MB/s, 22.5MB/s, 22.6MB/s, 22.6MB/s
With out ts collector : 22.5MB/s, 22.3MB/s, 22.6MB/s, 22.7MB/s, 22.6MB/s

The detailed results are as follows:
With ts collector :
5.193 micros/op 192573 ops/sec 22.8 MB/S
5.263 micros/op 190004 ops/sec 22.5 MB/S
5.266 micros/op 189885 ops/sec 22.5 MB/S
5.242 micros/op 190750 ops/sec 22.6 MB/S
5.228 micros/op 191274 ops/sec 22.6 MB/S

Without ts collector :
5.256 micros/op 190257 ops/sec 22.5 MB/s
5.298 micros/op 188735 ops/sec 22.3 MB/s
5.229 micros/op 191234 ops/sec 22.6 MB/s
5.204 micros/op 192158 ops/sec 22.7 MB/s
5.233 micros/op 191098 ops/sec 22.6 MB/s

riversand963

Thanks @sunlike-Lipo for the PR. Left some final minor comments before stamping.

riversand963 · 2021-11-19T04:05:49Z

db/table_properties_collector.h

+// internal key when Add() is invoked.
+//
+// @param cmp the Comparator to compare timestamp of key by the
+//     Comparator::CompareTimestamp function.


Not sure, but the indentation here seems weird.

riversand963 · 2021-11-19T04:08:56Z

db/table_properties_collector.h

+    properties->insert({"rocksdb.min_timestamp", min_timestamp_});
+    properties->insert({"rocksdb.max_timestamp", max_timestamp_});


I used sst_dump to dump the properties of generated SST files, and I can see

... # rocksdb.max_timestamp: 0x03000000000000000000000000000000 # rocksdb.merge.operands: 0x00 # rocksdb.min_timestamp: 0x03000000000000000000000000000000

This makes me wonder whether we can instead use rocksdb.timestamp_min and rocksdb.timestamp_max so that they can be next to each other.

riversand963 · 2021-11-19T04:09:44Z

db/table_properties_collector.h

+    return {{"rocksdb.min_timestamp", Slice(min_timestamp_).ToString(true)},
+            {"rocksdb.max_timestamp", Slice(max_timestamp_).ToString(true)}};


Same as previous one.
Can we instead use rocksdb.timestamp_min and rocksdb.timestamp_max so that they can be next to each other?

riversand963 · 2021-11-19T04:10:53Z

db/table_properties_collector.h

+  std::string max_timestamp_;
+  std::string min_timestamp_;


Should these two be explicitly initialized to kDisableTimestamp?

riversand963 · 2021-11-19T04:19:33Z

db/table_properties_collector.h

+        ExtractTimestampFromUserKey(user_key, cmp_->timestamp_size());
+    if (max_timestamp_ == kDisableUserTimestamp ||
+        cmp_->CompareTimestamp(timestamp_in_key, max_timestamp_) > 0) {
+      max_timestamp_ = timestamp_in_key.ToString();


If we do

max_timestamp_.assign(timestamp_in_key.data(), timestamp_in_key.size());

will it save us some string creation and copy caused by calling ToString()?

riversand963 · 2021-11-19T04:19:51Z

db/table_properties_collector.h

+    }
+    if (min_timestamp_ == kDisableUserTimestamp ||
+        cmp_->CompareTimestamp(min_timestamp_, timestamp_in_key) > 0) {
+      min_timestamp_ = timestamp_in_key.ToString();


riversand963 · 2021-11-19T04:20:44Z

db/table_properties_collector.h

+class TimestampTablePropertiesCollector : public IntTblPropCollector {
+ public:
+  explicit TimestampTablePropertiesCollector(const Comparator* cmp)
+      : cmp_(cmp) {}


Maybe here we can explicitly initialize max_timestamp_ and min_timestamp_.

facebook-github-bot · 2021-11-19T04:24:41Z

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

facebook-github-bot · 2021-11-19T06:39:49Z

@sunlike-Lipo has updated the pull request. You must reimport the pull request before landing.

sunlike-Lipo · 2021-11-19T09:25:20Z

@riversand963
I made some tests again after changing string = to string.assign

The db_bench result are as follows:
With ts collector :
5.175 micros/op 193250 ops/sec 22.9 MB/S
5.149 micros/op 194219 ops/sec 23.0 MB/S
5.223 micros/op 191443 ops/sec 22.6 MB/S
5.167 micros/op 193534 ops/sec 22.9 MB/S
5.150 micros/op 194183 ops/sec 23.0 MB/S

Without ts collector :
5.209 micros/op 191963 ops/sec 22.7 MB/s
5.201 micros/op 192256 ops/sec 22.7 MB/s
5.147 micros/op 194269 ops/sec 23.0 MB/s
5.230 micros/op 191211 ops/sec 22.6 MB/s
5.269 micros/op 189781 ops/sec 22.4 MB/s

The standalone test from sunlike-Lipo@50a1152 result are as follows:
use string.assign

	value_size=10	value_size=100	value_size=1000
collect-ts	3851ms	1219ms	202ms
not-collect-ts	3745ms	1181ms	189ms

use string.operator=

	value_size=10	value_size=100	value_size=1000
collect-ts	3868ms	1239ms	204ms
not-collect-ts	3767ms	1189ms	195ms

It seems there is little difference between the result using string.assign and using string.operator=, perhaps the compiler is
clever enough to optmize this.
Any way, I'll use string.assign because it ought to be faster than string.operator=.

facebook-github-bot · 2021-11-19T09:33:20Z

@sunlike-Lipo has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-11-19T17:56:52Z

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

facebook-github-bot · 2021-11-19T19:37:14Z

@riversand963 merged this pull request in e12753e.

facebook-github-bot added the CLA Signed label Oct 29, 2021

sunlike-Lipo changed the title ~~Track each SST's timestamp information as user properties #8959~~ Track each SST's timestamp information as user properties Oct 29, 2021

sunlike-Lipo force-pushed the develop branch 4 times, most recently from b5db9d7 to 7bdb3a6 Compare October 29, 2021 12:57

riversand963 mentioned this pull request Nov 4, 2021

Track per-SST user-defined timestamp information in MANIFEST #9092

Closed

riversand963 reviewed Nov 4, 2021

View reviewed changes

wolfkdy mentioned this pull request Nov 5, 2021

support rocksdb 6.x series mongodb-partners/mongo-rocks#164

Open

sunlike-Lipo force-pushed the develop branch 5 times, most recently from cdcdd89 to 2a1262e Compare November 8, 2021 00:55

Track each SST's timestamp information as user properties (facebook#8959

873c7ad

)

sunlike-Lipo force-pushed the develop branch from 2a1262e to 873c7ad Compare November 8, 2021 01:19

Track each SST's timestamp information as user properties (facebook#8959

5991279

)

sunlike-Lipo mentioned this pull request Nov 8, 2021

Track each SST's timestamp information as user properties #8959

Closed

riversand963 linked an issue Nov 12, 2021 that may be closed by this pull request

Track each SST's timestamp information as user properties #8959

Closed

riversand963 reviewed Nov 13, 2021

View reviewed changes

Merge branch 'facebook:main' into develop

57bfcd6

sunlike-Lipo force-pushed the develop branch from c43c600 to 9afe576 Compare November 15, 2021 12:33

sunlike-Lipo force-pushed the develop branch from a0e5b0f to 13ab3b6 Compare November 15, 2021 12:57

sunlike-Lipo force-pushed the develop branch from 13ab3b6 to 05ebb11 Compare November 15, 2021 13:11

sunlike-Lipo force-pushed the develop branch from 05ebb11 to c38de84 Compare November 15, 2021 13:33

fix review comments

886c942

sunlike-Lipo force-pushed the develop branch from c38de84 to 886c942 Compare November 15, 2021 14:27

Merge branch 'main' into develop

35f9154

riversand963 reviewed Nov 19, 2021

View reviewed changes

fix review comments

a7dfe15

sunlike-Lipo force-pushed the develop branch from 480400f to a7dfe15 Compare November 19, 2021 09:33

riversand963 approved these changes Nov 19, 2021

View reviewed changes

facebook-github-bot closed this in e12753e Nov 19, 2021

facebook-github-bot added the Merged label Nov 19, 2021

		properties->insert({"rocksdb.min_timestamp", min_timestamp_});
		properties->insert({"rocksdb.max_timestamp", max_timestamp_});

		return {{"rocksdb.min_timestamp", Slice(min_timestamp_).ToString(true)},
		{"rocksdb.max_timestamp", Slice(max_timestamp_).ToString(true)}};

Track each SST's timestamp information as user properties #9093

Track each SST's timestamp information as user properties #9093

Conversation

sunlike-Lipo commented Oct 29, 2021 • edited Loading

wolfkdy commented Nov 4, 2021

riversand963 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunlike-Lipo Nov 8, 2021 • edited Loading

Choose a reason for hiding this comment

wolfkdy commented Nov 4, 2021 • edited Loading

sunlike-Lipo commented Nov 11, 2021

riversand963 commented Nov 12, 2021

riversand963 left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 13, 2021

facebook-github-bot commented Nov 15, 2021

facebook-github-bot commented Nov 15, 2021

facebook-github-bot commented Nov 15, 2021

facebook-github-bot commented Nov 15, 2021

sunlike-Lipo commented Nov 15, 2021

facebook-github-bot commented Nov 15, 2021

facebook-github-bot commented Nov 15, 2021

facebook-github-bot commented Nov 15, 2021

sunlike-Lipo commented Nov 16, 2021 • edited Loading

facebook-github-bot commented Nov 16, 2021

riversand963 commented Nov 16, 2021

sunlike-Lipo commented Nov 17, 2021 • edited Loading

sunlike-Lipo commented Nov 18, 2021 • edited Loading

riversand963 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riversand963 Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riversand963 Nov 19, 2021 • 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

facebook-github-bot commented Nov 19, 2021

facebook-github-bot commented Nov 19, 2021

sunlike-Lipo commented Nov 19, 2021 • edited Loading

facebook-github-bot commented Nov 19, 2021

facebook-github-bot commented Nov 19, 2021

facebook-github-bot commented Nov 19, 2021

sunlike-Lipo commented Oct 29, 2021 •

edited

Loading

riversand963 left a comment •

edited

Loading

sunlike-Lipo Nov 8, 2021 •

edited

Loading

wolfkdy commented Nov 4, 2021 •

edited

Loading

sunlike-Lipo commented Nov 16, 2021 •

edited

Loading

sunlike-Lipo commented Nov 17, 2021 •

edited

Loading

sunlike-Lipo commented Nov 18, 2021 •

edited

Loading

riversand963 Nov 19, 2021 •

edited

Loading

riversand963 Nov 19, 2021 •

edited

Loading

sunlike-Lipo commented Nov 19, 2021 •

edited

Loading