Improve CPU Efficiency of ApproximateSize (part 1) #5613
Conversation
elipoz
commented
Jul 23, 2019
elipoz
commented
- Avoid creating the iterator in order to call BlockBasedTable::ApproximateOffsetOf(). Instead, directly call into it.
- Optimize BlockBasedTable::ApproximateOffsetOf() keeps the index block iterator in stack.
There was a problem hiding this comment.
@elipoz has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
elipoz
force-pushed
the
approximate-size-1
branch
from
58750bb
to
80aa5a1
Compare
|
@elipoz has updated the pull request. Re-import the pull request |
|
@elipoz has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@elipoz has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
db/table_cache.cc
Outdated
| @@ -505,4 +506,31 @@ void TableCache::Evict(Cache* cache, uint64_t file_number) { | |||
| cache->Erase(GetSliceForFileNumber(&file_number)); | |||
| } | |||
|
|
|||
| uint64_t TableCache::ApproximateSize(Version* v, const FdWithKeyRange& f, | |||
There was a problem hiding this comment.
We shouldn't pass Version here.
I believe there is TableCache::env_options_, and we need to pass in InternalComparator and prefix extractor separately.
elipoz
force-pushed
the
approximate-size-1
branch
from
80aa5a1
to
c902ea6
Compare
|
@elipoz has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@elipoz has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
db/table_cache.h
Outdated
| @@ -170,6 +171,11 @@ class TableCache { | |||
| } | |||
| } | |||
|
|
|||
| uint64_t ApproximateSize(const FdWithKeyRange& f, const Slice& key, | |||
There was a problem hiding this comment.
Need to add comment about what the function does. Also, does it makes sense to call it ApproximateOffsetOf() instead, considering that we are returning the file offset up to the key?
db/version_set.cc
Outdated
| delete iter; | ||
| TableReader* table_reader = f.file_metadata->fd.table_reader; | ||
| if (table_reader) { | ||
| result = table_reader->ApproximateOffsetOf(key, caller); |
There was a problem hiding this comment.
Can we put this call into TableCache::ApproximateSize too, just like other TableCache functions do?
There was a problem hiding this comment.
@siying is this call made from VersionSet::ApproximateSize always going to return not null?
TableCache* table_cache = v->cfd_->table_cache();
elipoz
force-pushed
the
approximate-size-1
branch
from
c902ea6
to
2e86674
Compare
|
@elipoz has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@elipoz has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
elipoz
force-pushed
the
approximate-size-1
branch
from
2e86674
to
8f52682
Compare
|
@elipoz has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@elipoz has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: 1. Avoid creating the iterator in order to call BlockBasedTable::ApproximateOffsetOf(). Instead, directly call into it. 2. Optimize BlockBasedTable::ApproximateOffsetOf() keeps the index block iterator in stack. Pull Request resolved: facebook#5613 Differential Revision: D16442660 Pulled By: elipoz fbshipit-source-id: 9320be3e918c139b10e758cbbb684706d172e516
