Skip to content

Avoid user key copying for Get/Put/Write with user-timestamp#5502

Closed
riversand963 wants to merge 7 commits intofacebook:masterfrom
riversand963:no_alloc_timestamp
Closed

Avoid user key copying for Get/Put/Write with user-timestamp#5502
riversand963 wants to merge 7 commits intofacebook:masterfrom
riversand963:no_alloc_timestamp

Conversation

@riversand963
Copy link
Copy Markdown
Contributor

@riversand963 riversand963 commented Jun 24, 2019

In previous #5079, we added user-specified timestamp to DB::Get() and DB::Put(). Limitation is that these two functions may cause extra memory allocation and key copy. The reason is that WriteBatch does not allocate extra memory for timestamps because it is not aware of timestamp size, and we did not provide an API to assign/update timestamp of each key within a WriteBatch.
We address these issues in this PR by doing the following.

  1. Add a timestamp_size_ to WriteBatch so that WriteBatch can take timestamps into account when calling WriteBatch::Put, WriteBatch::Delete, etc.
  2. Add APIs WriteBatch::AssignTimestamp and WriteBatch::AssignTimestamps so that application can assign/update timestamps for each key in a WriteBatch.
  3. Avoid key copy in GetImpl by adding new constructor to LookupKey.

Test plan (on devserver):

$make clean && COMPILE_WITH_ASAN=1 make -j32 all
$./db_basic_test --gtest_filter=Timestamp/DBBasicTestWithTimestampWithParam.PutAndGet/*
$make check

If the API extension looks good, I will add more unit tests.

Some simple benchmark using db_bench.

$rm -rf /dev/shm/dbbench/* && TEST_TMPDIR=/dev/shm ./db_bench -benchmarks=fillseq,readrandom -num=1000000
$rm -rf /dev/shm/dbbench/* && TEST_TMPDIR=/dev/shm ./db_bench -benchmarks=fillrandom -num=1000000 -disable_wal=true

Master is at a78503b.

|        | readrandom | fillrandom |
| master | 15.53 MB/s | 25.97 MB/s |
| PR5502 | 16.70 MB/s | 25.80 MB/s |

@siying
Copy link
Copy Markdown
Contributor

siying commented Jun 24, 2019

Please update the title to mention user timestamp.

@riversand963 riversand963 changed the title Avoid user key copying for Get/Put/Write [User timestamp] Avoid user key copying for Get/Put/Write Jun 24, 2019
@riversand963
Copy link
Copy Markdown
Contributor Author

Ping @siying

Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@riversand963 has updated the pull request. Re-import the pull request

@riversand963 riversand963 changed the title [User timestamp] Avoid user key copying for Get/Put/Write Avoid user key copying for Get/Put/Write with user-timestamp Jul 22, 2019
Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@siying siying left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@riversand963 has updated the pull request. Re-import the pull request

Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@riversand963 has updated the pull request. Re-import the pull request

Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@riversand963 riversand963 deleted the no_alloc_timestamp branch July 25, 2019 22:35
@facebook-github-bot
Copy link
Copy Markdown
Contributor

@riversand963 merged this pull request in ae152ee.

merryChris pushed a commit to merryChris/rocksdb that referenced this pull request Nov 18, 2019
…k#5502)

Summary:
In previous facebook#5079, we added user-specified timestamp to `DB::Get()` and `DB::Put()`. Limitation is that these two functions may cause extra memory allocation and key copy. The reason is that `WriteBatch` does not allocate extra memory for timestamps because it is not aware of timestamp size, and we did not provide an API to assign/update timestamp of each key within a `WriteBatch`.
We address these issues in this PR by doing the following.
1. Add a `timestamp_size_` to `WriteBatch` so that `WriteBatch` can take timestamps into account when calling `WriteBatch::Put`, `WriteBatch::Delete`, etc.
2. Add APIs `WriteBatch::AssignTimestamp` and `WriteBatch::AssignTimestamps` so that application can assign/update timestamps for each key in a `WriteBatch`.
3. Avoid key copy in `GetImpl` by adding new constructor to `LookupKey`.

Test plan (on devserver):
```
$make clean && COMPILE_WITH_ASAN=1 make -j32 all
$./db_basic_test --gtest_filter=Timestamp/DBBasicTestWithTimestampWithParam.PutAndGet/*
$make check
```
If the API extension looks good, I will add more unit tests.

Some simple benchmark using db_bench.
```
$rm -rf /dev/shm/dbbench/* && TEST_TMPDIR=/dev/shm ./db_bench -benchmarks=fillseq,readrandom -num=1000000
$rm -rf /dev/shm/dbbench/* && TEST_TMPDIR=/dev/shm ./db_bench -benchmarks=fillrandom -num=1000000 -disable_wal=true
```
Master is at a78503b.
```
|        | readrandom | fillrandom |
| master | 15.53 MB/s | 25.97 MB/s |
| PR5502 | 16.70 MB/s | 25.80 MB/s |
```
Pull Request resolved: facebook#5502

Differential Revision: D16340894

Pulled By: riversand963

fbshipit-source-id: 51132cf792be07d1efc3ac33f5768c4ee2608bb8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants