MultiCFIterator Refactor - CoalescingIterator & AttributeGroupIterator #12480

jaykorean · 2024-03-25T23:58:20Z

Summary

There are a couple of reasons to modify the current implementation of the MultiCfIterator, which implements the generic Iterator interface.

The default behavior of value()/columns() returning data from different Column Families for different keys can be prone to errors, even though there might be valid use cases where users do not care about the origin of the value/columns.
The attribute_groups() API, which is not yet implemented, will not be useful for a single-CF iterator.

In this PR, we are implementing the following changes:

IteratorBase introduced, which includes all basic iterator functions except value() and columns().
Iterator, which now inherits from IteratorBase, includes value() and columns().
New public interface AttributeGroupIterator inherits from IteratorBase and additionally includes attribute_groups() (to be implemented).
Renamed former MultiCfIterator to CoalescingIterator which inherits from Iterator
Existing MultiCfIteratorTest has been split into two - CoalescingIteratorTest and AttributeGroupIteratorTest.
Moved AttributeGroup related code from wide_columns.h to a new file, attribute_groups.h.

Some Implementation Details

MultiCfIteratorImpl takes two functions - populate_func and reset_func and use them to populate value_ and columns_ in CoalescingIterator and attribute_groups_ in AttributeGroupIterator. In CoalescingIterator, populate_func is Coalesce(), in AttributeGroupIterator populate_func is AddToAttributeGroups(). reset_func clears populated value_, columns_ and attribute_groups_ accordingly.
Coalesce() merge sorts columns from multiple CFs when a key exists in more than on CFs. column that appears in later CF overwrites the prior ones.

For example, if CF1 has "key_1" ==> {"col_1": "foo", "col_2", "baz"} and CF2 has "key_1" ==> {"col_2": "quux", "col_3", "bla"}, and when the iterator is at key_1, columns() will return {"col_1": "foo", "col_2", "quux", "col_3", "bla"}

In this example, value() will be empty, because none of them have values for kDefaultColumnName

Test Plan

Unit Test

./multi_cf_iterator_test

Performance Test

To make sure this change does not impact existing Iterator performance

Build

$> make -j64 release

Setup

$> TEST_TMPDIR=/dev/shm/db_bench ./db_bench -benchmarks="filluniquerandom" -key_size=32 -value_size=512 -num=1000000 -compression_type=none

Run

TEST_TMPDIR=/dev/shm/db_bench ./db_bench -use_existing_db=1 -benchmarks="newiterator,seekrandom" -cache_size=10485760000

Before the change

DB path: [/dev/shm/db_bench/dbbench]
newiterator  :       0.519 micros/op 1927904 ops/sec 0.519 seconds 1000000 operations;
DB path: [/dev/shm/db_bench/dbbench]
seekrandom   :       5.302 micros/op 188589 ops/sec 5.303 seconds 1000000 operations; (0 of 1000000 found)

After the change

DB path: [/dev/shm/db_bench/dbbench]
newiterator  :       0.497 micros/op 2011012 ops/sec 0.497 seconds 1000000 operations;
DB path: [/dev/shm/db_bench/dbbench]
seekrandom   :       5.252 micros/op 190405 ops/sec 5.252 seconds 1000000 operations; (0 of 1000000 found)

facebook-github-bot · 2024-03-26T03:43:51Z

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

facebook-github-bot · 2024-03-26T15:39:55Z

@jaykorean has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2024-03-26T15:42:00Z

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

ltamasi

The design looks great overall, thanks a lot @jaykorean ! Left some comments/questions below

db/attribute_group_iterator_impl.h

db/coalescing_iterator.h

db/db_impl/db_impl.cc

db/multi_cf_iterator_impl.h

include/rocksdb/attribute_groups.h

include/rocksdb/iterator_base.h

include/rocksdb/options.h

facebook-github-bot · 2024-03-30T00:27:27Z

@jaykorean has updated the pull request. You must reimport the pull request before landing.

db/coalescing_iterator.cc

jaykorean · 2024-03-30T00:31:38Z

db/coalescing_iterator.h

+
+namespace ROCKSDB_NAMESPACE {
+
+// UNDER CONSTRUCTION - DO NOT USE


I plan to lift this after adding this to stress_test

facebook-github-bot · 2024-03-30T01:04:06Z

@jaykorean has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2024-03-30T02:37:00Z

@jaykorean has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2024-04-01T16:22:47Z

@jaykorean has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2024-04-01T16:32:54Z

@jaykorean has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2024-04-01T16:43:21Z

@jaykorean has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2024-04-01T16:48:55Z

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

facebook-github-bot · 2024-04-01T17:53:10Z

@jaykorean has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2024-04-01T17:53:43Z

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

facebook-github-bot · 2024-04-01T19:51:31Z

@jaykorean has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2024-04-01T19:53:49Z

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

ltamasi

Thanks for the updates @jaykorean !! This looks awesome!

db/coalescing_iterator.cc

db/coalescing_iterator.h

ltamasi · 2024-04-09T17:31:29Z

db/multi_cf_iterator_impl.h

+      std::function<void()> reset_func,
+      std::function<void(ColumnFamilyHandle*, Iterator*)> populate_func)


Minor and we can do this as a follow-up but we can avoid the overhead of std::function by turning MultiCfIteratorImpl into a class template that takes the types of these two functors as template parameters.

I tried using the template first, but had a couple of issues that I ended up using std::function here.

The compiler was complaining with the following message and I still haven't figured out the reason.

./db/multi_cf_iterator_impl.h: In member function ‘void rocksdb::MultiCfIteratorImpl<ResetFuncType, PopulateFuncType>::InitMinHeap()’: ./db/multi_cf_iterator_impl.h:175:33: error: expected primary-expression before ‘>’ token 175 | heap_.emplace<MultiCfMinHeap>(

Minor, but in CoalescingIterator and AttributeGroupIterator, the impl had to contain the types of the two functions. e.g. MultiCfIteratorImpl<std::function<void()>, std::function<void(ColumnFamilyHandle*, Iterator*)>> impl_; which I found not as neaty/readable as I wanted it to be.

db/multi_cf_iterator_impl.h

include/rocksdb/attribute_groups.h

db/multi_cf_iterator_test.cc

facebook-github-bot · 2024-04-11T16:46:02Z

@jaykorean has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2024-04-11T16:48:34Z

@jaykorean has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2024-04-11T16:55:12Z

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

facebook-github-bot · 2024-04-11T18:39:52Z

@jaykorean merged this pull request in 58a98bd.

…on (#12534) Summary: Continuing from the previous MultiCfIterator Implementations - (#12422, #12480 #12465), this PR completes the `AttributeGroupIterator` by implementing `AttributeGroupIteratorImpl::AddToAttributeGroups()`. While implementing the `AttributeGroupIterator`, we had to make some changes in `MultiCfIteratorImpl` and found an opportunity to improve `Coalesce()` in `CoalescingIterator`. Lifting `UNDER CONSTRUCTION - DO NOT USE` comment by replacing it with `EXPERIMENTAL` Here are some implementation details: - `IteratorAttributeGroups` is introduced to avoid having to copy all `WideColumn` objects during iteration. - `PopulateIterator()` no longer advances non-top iterators that have the same key as the top iterator in the heap. - `AdvanceIterator()` needs to advance the non-top iterators when they have the same key as the top iterator in the heap. - Instead of populating one by one, `PopulateIterator()` now collects all items with the same key and calls `populate_func(items)` at once. - This allowed optimization in `Coalesce()` such that we no longer do K-1 rounds of 2-way merge, but do one K-way merge instead. Pull Request resolved: #12534 Test Plan: Uncommented the assertions in `verifyAttributeGroupIterator()` ``` ./multi_cf_iterator_test ``` Reviewed By: ltamasi Differential Revision: D56089019 Pulled By: jaykorean fbshipit-source-id: 6b0b4247e221f69b40b147d41492008cc9b15054

facebook-github-bot added the CLA Signed label Mar 25, 2024

jaykorean force-pushed the iterator_base branch from 6582451 to 0a9afc1 Compare March 26, 2024 00:01

jaykorean requested review from pdillinger and ltamasi March 26, 2024 00:03

jaykorean changed the title ~~[MultiCFIterator Refactor] Introduce Coalescing Iterator~~ MultiCFIterator Refactor - Introduce IteratorBase & Coalescing Iterator Mar 26, 2024

jaykorean changed the title ~~MultiCFIterator Refactor - Introduce IteratorBase & Coalescing Iterator~~ MultiCFIterator Refactor - Introduce IteratorBase & CoalescingIterator Mar 26, 2024

jaykorean force-pushed the iterator_base branch from 0a9afc1 to 2f211e3 Compare March 26, 2024 02:36

jaykorean changed the title ~~MultiCFIterator Refactor - Introduce IteratorBase & CoalescingIterator~~ MultiCFIterator Refactor - CoalescingIterator & AttributeGroupIterator Mar 26, 2024

jaykorean marked this pull request as ready for review March 26, 2024 03:43

ltamasi reviewed Mar 28, 2024

View reviewed changes

jaykorean force-pushed the iterator_base branch from d4a6df5 to e90568e Compare March 30, 2024 00:27

jaykorean commented Mar 30, 2024

View reviewed changes

db/coalescing_iterator.cc Show resolved Hide resolved

jaykorean commented Mar 30, 2024

View reviewed changes

jaykorean force-pushed the iterator_base branch from aed98be to 80e6409 Compare April 1, 2024 16:22

jaykorean force-pushed the iterator_base branch from 80e6409 to e01434a Compare April 1, 2024 16:32

jaykorean force-pushed the iterator_base branch from e01434a to 2a48a25 Compare April 1, 2024 16:43

jaykorean requested a review from ltamasi April 1, 2024 16:49

jaykorean force-pushed the iterator_base branch from 2a48a25 to 4f8fc31 Compare April 1, 2024 17:53

ltamasi approved these changes Apr 9, 2024

View reviewed changes

jaykorean added 12 commits April 11, 2024 08:36

Introduce IteratorBase

a16f2f9

Introduce CoalescingIterator and AttributeGroupIterator

5a1a7e3

Introduce CoalescingOptions and update test

43d5f29

Update Tests to separate Coalescing and AttributeGroup Iterator

edebbbe

AttributeGroupIterator as public interface

dfd230e

Test for AttributeGroupIterator

534ed1c

Attempt to fix linter

8233322

Address Feedback and implement merge

d7ee58e

get rid of unnecessary cfh pointer in MultiCfIteratorInfo

04af31f

fix unused var

523c068

fix linter

ad1734e

re-add changes lost

f912c6e

jaykorean force-pushed the iterator_base branch from e88536f to 24f0807 Compare April 11, 2024 16:45

Address feedback

46f50a1

jaykorean force-pushed the iterator_base branch from 24f0807 to 46f50a1 Compare April 11, 2024 16:48

This was referenced Apr 11, 2024

Introduce NewMultiCfIterator API #12153

Closed

Access DBImpl* and CFD* by CFHImpl* in Iterators #12395

Closed

facebook-github-bot closed this in 58a98bd Apr 11, 2024

facebook-github-bot added the Merged label Apr 11, 2024

jaykorean deleted the iterator_base branch April 11, 2024 20:26

jaykorean mentioned this pull request Apr 12, 2024

MultiCfIterator - AttributeGroupIter Impl & CoalescingIter Optimization #12534

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MultiCFIterator Refactor - CoalescingIterator & AttributeGroupIterator #12480

MultiCFIterator Refactor - CoalescingIterator & AttributeGroupIterator #12480

jaykorean commented Mar 25, 2024 •

edited

Loading

facebook-github-bot commented Mar 26, 2024

facebook-github-bot commented Mar 26, 2024

facebook-github-bot commented Mar 26, 2024

ltamasi left a comment

facebook-github-bot commented Mar 30, 2024

jaykorean Mar 30, 2024

facebook-github-bot commented Mar 30, 2024

facebook-github-bot commented Mar 30, 2024

facebook-github-bot commented Apr 1, 2024

facebook-github-bot commented Apr 1, 2024

facebook-github-bot commented Apr 1, 2024

facebook-github-bot commented Apr 1, 2024

facebook-github-bot commented Apr 1, 2024

facebook-github-bot commented Apr 1, 2024

facebook-github-bot commented Apr 1, 2024

facebook-github-bot commented Apr 1, 2024

ltamasi left a comment

ltamasi Apr 9, 2024

jaykorean Apr 11, 2024 •

edited

Loading

facebook-github-bot commented Apr 11, 2024

facebook-github-bot commented Apr 11, 2024

facebook-github-bot commented Apr 11, 2024

facebook-github-bot commented Apr 11, 2024


		namespace ROCKSDB_NAMESPACE {

		// UNDER CONSTRUCTION - DO NOT USE

		std::function<void()> reset_func,
		std::function<void(ColumnFamilyHandle, Iterator)> populate_func)

MultiCFIterator Refactor - CoalescingIterator & AttributeGroupIterator #12480

MultiCFIterator Refactor - CoalescingIterator & AttributeGroupIterator #12480

Conversation

jaykorean commented Mar 25, 2024 • edited Loading

Summary

Test Plan

Unit Test

Performance Test

facebook-github-bot commented Mar 26, 2024

facebook-github-bot commented Mar 26, 2024

facebook-github-bot commented Mar 26, 2024

ltamasi left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 30, 2024

jaykorean Mar 30, 2024

Choose a reason for hiding this comment

facebook-github-bot commented Mar 30, 2024

facebook-github-bot commented Mar 30, 2024

facebook-github-bot commented Apr 1, 2024

facebook-github-bot commented Apr 1, 2024

facebook-github-bot commented Apr 1, 2024

facebook-github-bot commented Apr 1, 2024

facebook-github-bot commented Apr 1, 2024

facebook-github-bot commented Apr 1, 2024

facebook-github-bot commented Apr 1, 2024

facebook-github-bot commented Apr 1, 2024

ltamasi left a comment

Choose a reason for hiding this comment

ltamasi Apr 9, 2024

Choose a reason for hiding this comment

jaykorean Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Apr 11, 2024

facebook-github-bot commented Apr 11, 2024

facebook-github-bot commented Apr 11, 2024

facebook-github-bot commented Apr 11, 2024

jaykorean commented Mar 25, 2024 •

edited

Loading

jaykorean Apr 11, 2024 •

edited

Loading