avoid `IterKey::UpdateInternalKey()` in `BlockIter` by ajkr · Pull Request #6843 · facebook/rocksdb

ajkr · 2020-05-13T23:11:17Z

IterKey::UpdateInternalKey() is an error-prone API as it's
incompatible with IterKey::TrimAppend(), which is used for
decoding delta-encoded internal keys. This PR stops using it in
BlockIter. Instead, it assigns global seqno in a separate IterKey's
buffer when needed. The logic for safely getting a Slice with global
seqno properly assigned is encapsulated in GlobalSeqnoAppliedKey.
BinarySeek() is also migrated to use this API (previously it ignored
global seqno entirely).

Test Plan:

benchmark setup -- single file DBs, in-memory, no compression. "normal_db"
created by regular flush; "ingestion_db" created by ingesting a file. Both
DBs have same contents.

$ TEST_TMPDIR=/dev/shm/normal_db/ ./db_bench -benchmarks=fillrandom,compact -write_buffer_size=10485760000 -disable_auto_compactions=true -compression_type=none -num=1000000
$ ./ldb write_extern_sst ./tmp.sst --db=/dev/shm/ingestion_db/dbbench/ --compression_type=no --hex --create_if_missing < <(./sst_dump --command=scan --output_hex --file=/dev/shm/normal_db/dbbench/000007.sst | awk 'began {print "0x" substr($1, 2, length($1) - 2), "==>", "0x" $5} ; /^Sst file format: block-based/ {began=1}')
$ ./ldb ingest_extern_sst ./tmp.sst --db=/dev/shm/ingestion_db/dbbench/

benchmark run command:

TEST_TMPDIR=/dev/shm/$DB/ ./db_bench -benchmarks=seekrandom -seek_nexts=10 -use_existing_db=true -cache_index_and_filter_blocks=false -num=1000000 -cache_size=1048576000 -threads=1 -reads=40000000

results:

DB	code	throughput
normal_db	master	267.9
normal_db	PR6843	254.2 (-5.1%)
ingestion_db	master	259.6
ingestion_db	PR6843	250.5 (-3.5%)

ajkr · 2020-05-13T23:19:23Z

There are a few next steps that were too hard to do all at once.

migrate IndexBlockIter to use GlobalSeqnoAppliedKey for first_internal_key
introduce a separate convenience class like IterKey for non-iterator keys. Currently IterKey is used in places unrelated to iteration/delta decoding (for example, GlobalSeqnoAppliedKey::scratch_).
finish migrating everything away from IterKey::UpdateInternalKey() and then delete it.

But this PR should be enough to unblock #6646...

facebook-github-bot

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

table/block_based/block.cc

facebook-github-bot · 2020-05-14T17:12:44Z

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

facebook-github-bot · 2020-05-14T17:51:22Z

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

facebook-github-bot

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

facebook-github-bot · 2020-05-15T01:04:40Z

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

facebook-github-bot · 2020-05-15T15:37:57Z

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

facebook-github-bot

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

pdillinger

Still digging deeper in my understanding, but sharing this partial feedback

table/block_based/block.h

facebook-github-bot · 2020-05-21T16:12:59Z

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

facebook-github-bot

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

pdillinger

Maybe also run a seekrandom benchmark (without cache_index_and_filter_blocks) to see what kind of worst-case performance impact this has.

table/block_based/block.cc

ajkr

Thanks for the review!

Maybe also run a seekrandom benchmark (without cache_index_and_filter_blocks) to see what kind of worst-case performance impact this has.

Sure, will do.

table/block_based/block.cc

facebook-github-bot · 2020-05-22T20:32:00Z

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

ajkr · 2020-05-22T20:37:50Z

Maybe also run a seekrandom benchmark (without cache_index_and_filter_blocks) to see what kind of worst-case performance impact this has.

Sure, will do.

Added to the description. The results aren't that stable, but I do consistently see slowdown in both the "ingestion_db" and "normal_db" case. That is different from what I expected as I thought the main drawback of this PR was introducing a copy into GlobalSeqnoAppliedKey::scratch_ only in the "ingestion_db" case.

pdillinger

I think we can accept minor perf regression to help bring some sanity to this part of the code.

pdillinger · 2020-05-26T17:08:43Z

(I believe the CI failures are all unrelated. Do another rebase if you want.)

`IterKey::UpdateInternalKey()` is an error-prone API as it's incompatible with `IterKey::TrimAppend()`, which is used for decoding delta-encoded internal keys. This PR stops using it in `BlockIter`. Instead, it assigns global seqno in a separate `IterKey`'s buffer when needed. The logic for safely getting a Slice with global seqno properly assigned is encapsulated in `GlobalSeqnoAppliedKey`. `BinarySeek()` is also migrated to use this API (previously it ignored global seqno entirely). Test Plan: benchmark setup -- single file DBs, in-memory, no compression. "normal_db" created by regular flush; "ingestion_db" created by ingesting a file. Both DBs have same contents. ``` $ TEST_TMPDIR=/dev/shm/normal_db/ ./db_bench -benchmarks=fillrandom,compact -write_buffer_size=10485760000 -disable_auto_compactions=true -compression_type=none -num=1000000 $ ./ldb write_extern_sst ./tmp.sst --db=/dev/shm/ingestion_db/dbbench/ --compression_type=no --hex --create_if_missing < <(./sst_dump --command=scan --output_hex --file=/dev/shm/normal_db/dbbench/000007.sst | awk 'began {print "0x" substr($1, 2, length($1) - 2), "==>", "0x" $5} ; /^Sst file format: block-based/ {began=1}') $ ./ldb ingest_extern_sst ./tmp.sst --db=/dev/shm/ingestion_db/dbbench/ ``` benchmark run command: ``` TEST_TMPDIR=/dev/shm/$DB/ ./db_bench -benchmarks=seekrandom -seek_nexts=10 -use_existing_db=true -cache_index_and_filter_blocks=false -num=1000000 -cache_size=1048576000 -threads=1 -reads=40000000 ``` results: DB code throughput normal_db master 267.9 normal_db PR6843 254.2 (-5.1%) ingestion_db master 259.6 ingestion_db PR6843 250.5 (-3.5%)

yiwu-arbug

Thanks for the fix.

facebook-github-bot · 2020-05-28T02:04:15Z

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

ajkr · 2020-05-28T02:04:50Z

Thanks for the review!

I think we can accept minor perf regression to help bring some sanity to this part of the code.

The "normal_db" regression can be avoided if we change UpdateAndGetKey() to assume it holds non-ingested keys (i.e., if (global_seqno_ == kDisableGlobalSequenceNumber) -> if (LIKELY(global_seqno_ == kDisableGlobalSequenceNumber))). It's not obvious to me we should do that so I'll leave it as is for now.

The "ingestion_db" regression is due to the extra copy into GlobalSeqnoAppliedKey::scratch_ that can only be avoided by comparing against ParsedInternalKey. More refactoring is necessary first.

facebook-github-bot

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

facebook-github-bot · 2020-05-28T18:44:42Z

@ajkr merged this pull request in c5abf78.

facebook-github-bot · 2020-05-28T18:45:00Z

@ajkr merged this pull request in c5abf78.

…175) Summary: `IterKey::UpdateInternalKey()` is an error-prone API as it's incompatible with `IterKey::TrimAppend()`, which is used for decoding delta-encoded internal keys. This PR stops using it in `BlockIter`. Instead, it assigns global seqno in a separate `IterKey`'s buffer when needed. The logic for safely getting a Slice with global seqno properly assigned is encapsulated in `GlobalSeqnoAppliedKey`. `BinarySeek()` is also migrated to use this API (previously it ignored global seqno entirely). Pull Request resolved: facebook#6843 Test Plan: benchmark setup -- single file DBs, in-memory, no compression. "normal_db" created by regular flush; "ingestion_db" created by ingesting a file. Both DBs have same contents. ``` $ TEST_TMPDIR=/dev/shm/normal_db/ ./db_bench -benchmarks=fillrandom,compact -write_buffer_size=10485760000 -disable_auto_compactions=true -compression_type=none -num=1000000 $ ./ldb write_extern_sst ./tmp.sst --db=/dev/shm/ingestion_db/dbbench/ --compression_type=no --hex --create_if_missing < <(./sst_dump --command=scan --output_hex --file=/dev/shm/normal_db/dbbench/000007.sst | awk 'began {print "0x" substr($1, 2, length($1) - 2), "==>", "0x" $5} ; /^Sst file format: block-based/ {began=1}') $ ./ldb ingest_extern_sst ./tmp.sst --db=/dev/shm/ingestion_db/dbbench/ ``` benchmark run command: ``` TEST_TMPDIR=/dev/shm/$DB/ ./db_bench -benchmarks=seekrandom -seek_nexts=10 -use_existing_db=true -cache_index_and_filter_blocks=false -num=1000000 -cache_size=1048576000 -threads=1 -reads=40000000 ``` results: | DB | code | throughput | |---|---|---| | normal_db | master | 267.9 | | normal_db | PR6843 | 254.2 (-5.1%) | | ingestion_db | master | 259.6 | | ingestion_db | PR6843 | 250.5 (-3.5%) | Reviewed By: pdillinger Differential Revision: D21562604 Pulled By: ajkr fbshipit-source-id: 937596f836930515da8084d11755e1f247dcb264 Signed-off-by: Yi Wu <yiwu@pingcap.com> Co-authored-by: Andrew Kryczka <andrewkr@fb.com>

…174) Summary: `IterKey::UpdateInternalKey()` is an error-prone API as it's incompatible with `IterKey::TrimAppend()`, which is used for decoding delta-encoded internal keys. This PR stops using it in `BlockIter`. Instead, it assigns global seqno in a separate `IterKey`'s buffer when needed. The logic for safely getting a Slice with global seqno properly assigned is encapsulated in `GlobalSeqnoAppliedKey`. `BinarySeek()` is also migrated to use this API (previously it ignored global seqno entirely). Pull Request resolved: facebook#6843 Test Plan: benchmark setup -- single file DBs, in-memory, no compression. "normal_db" created by regular flush; "ingestion_db" created by ingesting a file. Both DBs have same contents. ``` $ TEST_TMPDIR=/dev/shm/normal_db/ ./db_bench -benchmarks=fillrandom,compact -write_buffer_size=10485760000 -disable_auto_compactions=true -compression_type=none -num=1000000 $ ./ldb write_extern_sst ./tmp.sst --db=/dev/shm/ingestion_db/dbbench/ --compression_type=no --hex --create_if_missing < <(./sst_dump --command=scan --output_hex --file=/dev/shm/normal_db/dbbench/000007.sst | awk 'began {print "0x" substr($1, 2, length($1) - 2), "==>", "0x" $5} ; /^Sst file format: block-based/ {began=1}') $ ./ldb ingest_extern_sst ./tmp.sst --db=/dev/shm/ingestion_db/dbbench/ ``` benchmark run command: ``` TEST_TMPDIR=/dev/shm/$DB/ ./db_bench -benchmarks=seekrandom -seek_nexts=10 -use_existing_db=true -cache_index_and_filter_blocks=false -num=1000000 -cache_size=1048576000 -threads=1 -reads=40000000 ``` results: | DB | code | throughput | |---|---|---| | normal_db | master | 267.9 | | normal_db | PR6843 | 254.2 (-5.1%) | | ingestion_db | master | 259.6 | | ingestion_db | PR6843 | 250.5 (-3.5%) | Reviewed By: pdillinger Differential Revision: D21562604 Pulled By: ajkr fbshipit-source-id: 937596f836930515da8084d11755e1f247dcb264 Signed-off-by: Yi Wu <yiwu@pingcap.com> Co-authored-by: Andrew Kryczka <andrewkr@fb.com>

…173) Summary: `IterKey::UpdateInternalKey()` is an error-prone API as it's incompatible with `IterKey::TrimAppend()`, which is used for decoding delta-encoded internal keys. This PR stops using it in `BlockIter`. Instead, it assigns global seqno in a separate `IterKey`'s buffer when needed. The logic for safely getting a Slice with global seqno properly assigned is encapsulated in `GlobalSeqnoAppliedKey`. `BinarySeek()` is also migrated to use this API (previously it ignored global seqno entirely). Pull Request resolved: facebook#6843 Test Plan: benchmark setup -- single file DBs, in-memory, no compression. "normal_db" created by regular flush; "ingestion_db" created by ingesting a file. Both DBs have same contents. ``` $ TEST_TMPDIR=/dev/shm/normal_db/ ./db_bench -benchmarks=fillrandom,compact -write_buffer_size=10485760000 -disable_auto_compactions=true -compression_type=none -num=1000000 $ ./ldb write_extern_sst ./tmp.sst --db=/dev/shm/ingestion_db/dbbench/ --compression_type=no --hex --create_if_missing < <(./sst_dump --command=scan --output_hex --file=/dev/shm/normal_db/dbbench/000007.sst | awk 'began {print "0x" substr($1, 2, length($1) - 2), "==>", "0x" $5} ; /^Sst file format: block-based/ {began=1}') $ ./ldb ingest_extern_sst ./tmp.sst --db=/dev/shm/ingestion_db/dbbench/ ``` benchmark run command: ``` TEST_TMPDIR=/dev/shm/$DB/ ./db_bench -benchmarks=seekrandom -seek_nexts=10 -use_existing_db=true -cache_index_and_filter_blocks=false -num=1000000 -cache_size=1048576000 -threads=1 -reads=40000000 ``` results: | DB | code | throughput | |---|---|---| | normal_db | master | 267.9 | | normal_db | PR6843 | 254.2 (-5.1%) | | ingestion_db | master | 259.6 | | ingestion_db | PR6843 | 250.5 (-3.5%) | Reviewed By: pdillinger Differential Revision: D21562604 Pulled By: ajkr fbshipit-source-id: 937596f836930515da8084d11755e1f247dcb264 Signed-off-by: Yi Wu <yiwu@pingcap.com> Co-authored-by: Andrew Kryczka <andrewkr@fb.com>

facebook-github-bot added the CLA Signed label May 13, 2020

facebook-github-bot reviewed May 13, 2020

View reviewed changes

ajkr requested review from pdillinger and siying May 13, 2020 23:21

ajkr commented May 14, 2020

View reviewed changes

table/block_based/block.cc Outdated Show resolved Hide resolved

ajkr force-pushed the refactor-data-block-global-seqno branch from 2a280ed to 50f8d5f Compare May 14, 2020 17:12

ajkr force-pushed the refactor-data-block-global-seqno branch from 50f8d5f to 8f0dd79 Compare May 14, 2020 17:49

facebook-github-bot reviewed May 14, 2020

View reviewed changes

ajkr force-pushed the refactor-data-block-global-seqno branch from 8f0dd79 to 9c3e78e Compare May 15, 2020 01:04

ajkr force-pushed the refactor-data-block-global-seqno branch from 9c3e78e to cdd9c22 Compare May 15, 2020 15:37

facebook-github-bot reviewed May 15, 2020

View reviewed changes

ajkr removed the request for review from siying May 20, 2020 17:35

pdillinger requested changes May 20, 2020

View reviewed changes

table/block_based/block.h Outdated Show resolved Hide resolved

ajkr force-pushed the refactor-data-block-global-seqno branch from cdd9c22 to 5cdb42d Compare May 21, 2020 16:12

facebook-github-bot reviewed May 21, 2020

View reviewed changes

pdillinger reviewed May 21, 2020

View reviewed changes

table/block_based/block.cc Outdated Show resolved Hide resolved

ajkr commented May 22, 2020

View reviewed changes

table/block_based/block.cc Outdated Show resolved Hide resolved

ajkr force-pushed the refactor-data-block-global-seqno branch from 5cdb42d to 82b2f26 Compare May 22, 2020 20:31

pdillinger approved these changes May 26, 2020

View reviewed changes

ajkr mentioned this pull request May 27, 2020

Fix wrong result read from ingested file from iterator #6882

Closed

yiwu-arbug reviewed May 28, 2020

View reviewed changes

ajkr force-pushed the refactor-data-block-global-seqno branch from 82b2f26 to 089f675 Compare May 28, 2020 02:03

facebook-github-bot reviewed May 28, 2020

View reviewed changes

facebook-github-bot closed this in c5abf78 May 28, 2020

facebook-github-bot added the Merged label May 28, 2020

This was referenced Jun 3, 2020

avoid IterKey::UpdateInternalKey() in BlockIter (#6843) tikv/rocksdb#173

Merged

avoid IterKey::UpdateInternalKey() in BlockIter (#6843) tikv/rocksdb#174

Merged

avoid IterKey::UpdateInternalKey() in BlockIter (#6843) tikv/rocksdb#175

Merged

petermattis mentioned this pull request Jun 13, 2020

storage: investigate whether facebook/rocksdb#6843 should be backported cockroachdb/cockroach#50183

Closed

ajkr mentioned this pull request Jun 30, 2020

Separate internal and user key comparators in BlockIter #6944

Closed

Conversation

ajkr commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajkr commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

facebook-github-bot commented May 14, 2020

Uh oh!

facebook-github-bot commented May 14, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented May 15, 2020

Uh oh!

facebook-github-bot commented May 15, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

pdillinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

facebook-github-bot commented May 21, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

pdillinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ajkr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

facebook-github-bot commented May 22, 2020

Uh oh!

ajkr commented May 22, 2020

Uh oh!

pdillinger left a comment

Choose a reason for hiding this comment

Uh oh!

pdillinger commented May 26, 2020

Uh oh!

yiwu-arbug left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented May 28, 2020

Uh oh!

ajkr commented May 28, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented May 28, 2020

Uh oh!

facebook-github-bot commented May 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ajkr commented May 13, 2020 •

edited

Loading

ajkr commented May 13, 2020 •

edited

Loading