-
Notifications
You must be signed in to change notification settings - Fork 872
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(dbslice): Add self-laundering iterator in DbSlice
#2815
Conversation
src/server/db_slice.h
Outdated
private: | ||
void Set(std::string s, std::string_view sv); | ||
|
||
std::string s_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice. why not variant, btw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Move this, use it in all places we have variant<string, string_view>
Why do we need a big custom class if a simple wrapper is enough? 🤷🏻♂️ we have something similar here:
Lines 102 to 118 in a93ad4e
struct Key { | |
operator std::string_view() const { | |
return visit([](const auto& s) -> std::string_view { return s; }, val_); | |
} | |
bool operator==(const Key& o) const { | |
return *this == std::string_view(o); | |
} | |
friend std::ostream& operator<<(std::ostream& o, const Key& key) { | |
return o << std::string_view(key); | |
} | |
// If the key is backed by a string_view, replace it with a string with the same value | |
void MakeOwned() const; | |
mutable std::variant<std::string_view, std::string> val_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol, this is WIP, I didn't mean for anyone to review it yet :)
Re/ variant: I wanted to see an implementation with both members, where s_
is only used as a storage medium, and sv_
is always used for inspecting the string. But I agree that variant
is better (except for an added if
in the visit
path, but that's super unimportant and premature optimization).
Re/ big custom class - I'm not sure what you mean by big, but the Key
you linked to is also a "custom class" 🤣 It's just a private class that's not usable outside of table, and is one of this things that should be replaced by a general utility.
Anyway, I moved the StringOrView
class to a standalone file, took the variant
approach like Key
and moved table.h to use it.
But, as I said, this is still WIP, so unless you are really bored, I wouldn't look at this PR yet 🤓
src/server/db_slice.h
Outdated
private: | ||
void Set(std::string s, std::string_view sv); | ||
|
||
std::string s_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Move this, use it in all places we have variant<string, string_view>
Why do we need a big custom class if a simple wrapper is enough? 🤷🏻♂️ we have something similar here:
Lines 102 to 118 in a93ad4e
struct Key { | |
operator std::string_view() const { | |
return visit([](const auto& s) -> std::string_view { return s; }, val_); | |
} | |
bool operator==(const Key& o) const { | |
return *this == std::string_view(o); | |
} | |
friend std::ostream& operator<<(std::ostream& o, const Key& key) { | |
return o << std::string_view(key); | |
} | |
// If the key is backed by a string_view, replace it with a string with the same value | |
void MakeOwned() const; | |
mutable std::variant<std::string_view, std::string> val_; |
DbSlice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't guarantee that every replacement was correct 😅 But the ones I looked at were ok
src/server/db_slice.h
Outdated
// Auto-laundering iterator wrapper. | ||
template <typename T> class IteratorT { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest adding a very short comment what laundering means so it's show in code-completion snippets
auto [it, inserted] = locks_.try_emplace(Key{key}); | ||
if (!inserted) // If more than one transaction refers to a key | ||
it->first.MakeOwned(); // we must fall back to using a self-contained string | ||
auto [it, inserted] = locks_.try_emplace(Key::FromView(key)); | ||
if (!inserted) // If more than one transaction refers to a key | ||
const_cast<Key&>(it->first).MakeOwned(); // we must fall back to using a self-contained string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... seems legit 🤷🏻♂️ I don't think the hash table stores the keys as const in any place because it has at least to move out of them to move them around
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw it's exactly the same as before, except that now MakeOwned()
is non-const
(because it's not a const
operation), and we do the const_cast
because know that it wouldn't matter
|
||
using Iterator = IteratorT<PrimeIterator>; | ||
using ConstIterator = IteratorT<PrimeConstIterator>; | ||
using ExpIterator = IteratorT<ExpireIterator>; | ||
using ExpConstIterator = IteratorT<ExpireConstIterator>; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just typename T seems to be too general for structures that actually differ only verly slightly, it's always typename DashTable<K, V, P>::template Iterator<C, S>
. It's possible to infer them, but that's very template-heavy code so not sure it's worth
@@ -242,7 +242,8 @@ bool SliceSnapshot::BucketSaveCb(PrimeIterator it) { | |||
++stats_.skipped; | |||
return false; | |||
} | |||
db_slice_->FlushChangeToEarlierCallbacks(current_db_, it, snapshot_version_); | |||
db_slice_->FlushChangeToEarlierCallbacks(current_db_, DbSlice::Iterator::FromPrime(it), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to call DbSlice::Iterator::FromPrime here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FlushChangeToEarlierCallbacks
calls pre-update callbacks. I think (but I'm not sure) that we will need to give them the auto-laundering iterator as well, as they might also yield.
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is not the whole motivation of this PR is to allow SliceSnapshot::OnDbChange
to yield?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely want OnDbChange to be able to yield. And I think it's a good idea that it will have an auto laundering iterator to make things easier there. But it's not a necessity as the callbacks could launder automatically and anyway they should be using the values and not find etc.
but again, I think it's a good idea.
src/core/string_or_view.cc
Outdated
return sov; | ||
} | ||
|
||
bool StringOrView::operator==(const StringOrView& o) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: i would move onelines to the class definition and others to the header file and get rid of cc.
@@ -288,8 +288,8 @@ void Renamer::Find(Transaction* t) { | |||
auto& db_slice = EngineShard::tlocal()->db_slice(); | |||
auto [it, exp_it] = db_slice.FindReadOnly(t->GetDbContext(), res->key); | |||
|
|||
res->found = IsValid(it); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you could provide IsValid
override in db_slice.h and greatly reduce the size of the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, this is why it existed in the first place - I had different type of iterators in the past and wanted to handle them "cleanly".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, i forgot about this comment. Please do not review yet, I'll fix this as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :)
src/server/db_slice.h
Outdated
} | ||
|
||
private: | ||
void LaunderIfNeeded() const { // const is a lie |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: to pull the definition below DbSlice declaration.
src/server/db_slice.h
Outdated
|
||
uint64_t current_epoch = util::fb2::FiberSwitchEpoch(); | ||
if (current_epoch != fiber_epoch_) { | ||
if (it_->first != key_.view()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a note we currently do not shrink dashtables, but we do, we must also check if "it.egment_id: falls into a valid range.
@romange / @adiholden friendly ping :) |
src/server/db_slice.h
Outdated
@@ -330,12 +391,13 @@ class DbSlice { | |||
} | |||
|
|||
// Check whether 'it' has not expired. Returns it if it's still valid. Otherwise, erases it | |||
// from both tables and return PrimeIterator{}. | |||
// from both tables and return Iterator{}. | |||
struct ItAndExp { | |||
PrimeIterator it; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldnt we change this as well PrimeIterator -> Iterator and ExpireIterator -> ExpIterator ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is, we call this also in places where we have just PrimeIterator
in our hands, like when we search directly in the table (not through DbSlice). I thought it would be better to leave as is, as it won't have any benefit to change.
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well its confusing and can lead to bugs but I think we can revisit this later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give an example for a bug it could lead to?
Because it's strongly typed, I'm not sure what you think of. I am happy to change it now instead of later though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have places in which we call ExpireIfNeeded when we call with Iterator and not PrimeIterator and than we will do another Find call. I believe we dont have such cases right now but the fact that we do such api that we enable calling ExpireIfNeeded with Iterator can lead to such bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that makes sense. Done.
src/server/generic_family.cc
Outdated
@@ -1500,7 +1500,7 @@ OpStatus GenericFamily::OpMove(const OpArgs& op_args, string_view key, DbIndex t | |||
db_slice.ActivateDb(target_db); | |||
|
|||
bool sticky = from_res.it->first.IsSticky(); | |||
uint64_t exp_ts = db_slice.ExpireTime(from_res.exp_it); | |||
uint64_t exp_ts = db_slice.ExpireTime(from_res.exp_it.GetInnerIt()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that you can call db_slice.ExpireTime(from_res.exp_it); right?
src/server/string_family.cc
Outdated
@@ -531,7 +529,7 @@ SinkReplyBuilder::MGetResponse OpMGet(bool fetch_mcflag, bool fetch_mcver, const | |||
} | |||
|
|||
if (fetch_mcver) { | |||
resp.mc_ver = it.GetVersion(); | |||
resp.mc_ver = it.GetInnerIt().GetVersion(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it->GetVersion()
src/server/db_slice.h
Outdated
// falls into a valid range. | ||
uint64_t current_epoch = util::fb2::FiberSwitchEpoch(); | ||
if (current_epoch != fiber_epoch_) { | ||
if (it_->first != key_.view()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!it->IsOccupied() || it_->first != key_.view()
@@ -605,7 +609,7 @@ template <typename T> void DbSlice::IteratorT<T>::LaunderIfNeeded() const { | |||
// falls into a valid range. | |||
uint64_t current_epoch = util::fb2::FiberSwitchEpoch(); | |||
if (current_epoch != fiber_epoch_) { | |||
if (it_->first != key_.view()) { | |||
if (!it_.IsOccupied() || it_->first != key_.view()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now you can remove the above comment as IsOccupied will check if we still fall into valid range
…nfly ( v1.16.1 → v1.17.0 ) (#3473) This PR contains the following updates: | Package | Update | Change | |---|---|---| | [docker.dragonflydb.io/dragonflydb/dragonfly](https://togithub.com/dragonflydb/dragonfly) | minor | `v1.16.1` -> `v1.17.0` | --- ### Release Notes <details> <summary>dragonflydb/dragonfly (docker.dragonflydb.io/dragonflydb/dragonfly)</summary> ### [`v1.17.0`](https://togithub.com/dragonflydb/dragonfly/releases/tag/v1.17.0) [Compare Source](https://togithub.com/dragonflydb/dragonfly/compare/v1.16.1...v1.17.0) ##### Dragonfly v1.17.0 Some prominent changes include: - Improved performance for MGET operations ([#​2453](https://togithub.com/dragonflydb/dragonfly/issues/2453)) - Fix argument parsing in json.objkeys ([#​2872](https://togithub.com/dragonflydb/dragonfly/issues/2872)) - Fix ipv6 support for replication ([#​2889](https://togithub.com/dragonflydb/dragonfly/issues/2889)) - Support serialisation of bloom filters - saving to and loading from snapshots ([#​2846](https://togithub.com/dragonflydb/dragonfly/issues/2846)) - Support of HLL PFADD ([#​2761](https://togithub.com/dragonflydb/dragonfly/issues/2761)) - Support bullmq workloads that do not have `{}` hashtags in their queue names ([#​2890](https://togithub.com/dragonflydb/dragonfly/issues/2890)) ##### What's Changed - fix: [#​2745](https://togithub.com/dragonflydb/dragonfly/issues/2745) don't start migration process again after apply the same the same config is applied by [@​BorysTheDev](https://togithub.com/BorysTheDev) in [dragonflydb/dragonfly#2822 - feat(transaction): Idempotent callbacks (immediate runs) by [@​dranikpg](https://togithub.com/dranikpg) in [dragonflydb/dragonfly#2453 - refactor(cluster): replace sync_id with node_id for slot migration [#​2835](https://togithub.com/dragonflydb/dragonfly/issues/2835) by [@​BorysTheDev](https://togithub.com/BorysTheDev) in [dragonflydb/dragonfly#2838 - feat(tiering): Simple OpManager by [@​dranikpg](https://togithub.com/dranikpg) in [dragonflydb/dragonfly#2781 - chore: implement path mutation for JsonFlat by [@​romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2805 - feat(cluster): add migration removing by config [#​2835](https://togithub.com/dragonflydb/dragonfly/issues/2835) by [@​BorysTheDev](https://togithub.com/BorysTheDev) in [dragonflydb/dragonfly#2844 - chore: expose direct API on Bloom objects by [@​romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2845 - chore: generalize CompactObject::AllocateMR by [@​kostasrim](https://togithub.com/kostasrim) in [dragonflydb/dragonfly#2847 - feat(tiering): Simplest small bins by [@​dranikpg](https://togithub.com/dranikpg) in [dragonflydb/dragonfly#2810 - refactor: clean cluster slot migration code by [@​BorysTheDev](https://togithub.com/BorysTheDev) in [dragonflydb/dragonfly#2848 - fix(tests): Fix numsub test by [@​dranikpg](https://togithub.com/dranikpg) in [dragonflydb/dragonfly#2852 - fix: healthcheck for docker containers by [@​romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2853 - fix: possible crash in tls code by [@​romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2854 - fix(server): Do not block admin-port commands by [@​chakaz](https://togithub.com/chakaz) in [dragonflydb/dragonfly#2842 - fix(pytest): make pytests fail if server crash on shutdown by [@​adiholden](https://togithub.com/adiholden) in [dragonflydb/dragonfly#2827 - feat(server): add prints on takeover timeout by [@​adiholden](https://togithub.com/adiholden) in [dragonflydb/dragonfly#2856 - fix(pytest): dont check process return code on kill by [@​adiholden](https://togithub.com/adiholden) in [dragonflydb/dragonfly#2862 - fix: authorize the http connection to call commands by [@​romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2863 - feat(cluster): Send number of keys for incoming and outgoing migrations. by [@​chakaz](https://togithub.com/chakaz) in [dragonflydb/dragonfly#2858 - feat(tiering): TieredStorageV2 by [@​dranikpg](https://togithub.com/dranikpg) in [dragonflydb/dragonfly#2849 - bug(server): set connection flags block/pause flag on all blocking commands by [@​adiholden](https://togithub.com/adiholden) in [dragonflydb/dragonfly#2816 - chore: serialize SBF by [@​romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2846 - fix: test_replicaof_reject_on_load crash on stop by [@​kostasrim](https://togithub.com/kostasrim) in [dragonflydb/dragonfly#2818 - feat(dbslice): Add self-laundering iterator in `DbSlice` by [@​chakaz](https://togithub.com/chakaz) in [dragonflydb/dragonfly#2815 - chore: License update by [@​romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2767 - fix(acl): incompatibilities with acl load by [@​kostasrim](https://togithub.com/kostasrim) in [dragonflydb/dragonfly#2867 - fix(json): make path optional in json.objkeys by [@​kostasrim](https://togithub.com/kostasrim) in [dragonflydb/dragonfly#2872 - fix: return wrong type errors for SET...GET command by [@​romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2874 - fix(redis replication): remove partial sync flow ,not supported yet by [@​adiholden](https://togithub.com/adiholden) in [dragonflydb/dragonfly#2865 - chore: limit traffic logger only to the main interface by [@​romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2877 - chore: relax repltakeover constraints to only exclude write commands by [@​kostasrim](https://togithub.com/kostasrim) in [dragonflydb/dragonfly#2873 - chore(replayer): Roll back to go1.18 by [@​dranikpg](https://togithub.com/dranikpg) in [dragonflydb/dragonfly#2881 - fix: brpoplpush single shard to wake up blocked transactions by [@​kostasrim](https://togithub.com/kostasrim) in [dragonflydb/dragonfly#2875 - chore: LockTable tracks fingerprints of keys by [@​romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2839 - chore: reject TLS handshake when our listener is plain TCP by [@​romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2882 - Add support for Sparse HLL PFADD by [@​azuredream](https://togithub.com/azuredream) in [dragonflydb/dragonfly#2761 - feat server: bring visibility to script errors by [@​adiholden](https://togithub.com/adiholden) in [dragonflydb/dragonfly#2879 - chore: clean up REPLTAKEOVER flow by [@​romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2887 - chore(tiering): Move files and move kb literal to common by [@​dranikpg](https://togithub.com/dranikpg) in [dragonflydb/dragonfly#2868 - chore(interpreter): Support object replies by [@​dranikpg](https://togithub.com/dranikpg) in [dragonflydb/dragonfly#2885 - fix(ci/helm): Stick to v0.73.0 version of prom operator by [@​Pothulapati](https://togithub.com/Pothulapati) in [dragonflydb/dragonfly#2893 - fix(acl): authentication with UDS socket by [@​kostasrim](https://togithub.com/kostasrim) in [dragonflydb/dragonfly#2895 - feat(cluster): add repeated ACK if an error is happened by [@​BorysTheDev](https://togithub.com/BorysTheDev) in [dragonflydb/dragonfly#2892 - chore(blocking): Remove faulty DCHECK by [@​dranikpg](https://togithub.com/dranikpg) in [dragonflydb/dragonfly#2898 - chore: add a clear link on how to build dragonfly from source by [@​romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2884 - feat(server): Allow configuration of hashtag extraction by [@​chakaz](https://togithub.com/chakaz) in [dragonflydb/dragonfly#2890 - fix: fix build under macos by [@​BorysTheDev](https://togithub.com/BorysTheDev) in [dragonflydb/dragonfly#2901 - fix(cluster_replication): replicate redis cluster node bug fix by [@​adiholden](https://togithub.com/adiholden) in [dragonflydb/dragonfly#2876 - fix(acl): skip http and add check on connection traversals by [@​kostasrim](https://togithub.com/kostasrim) in [dragonflydb/dragonfly#2883 - fix(zset): Better memory consumption calculation by [@​chakaz](https://togithub.com/chakaz) in [dragonflydb/dragonfly#2900 - fix: fix ld for num converting by [@​BorysTheDev](https://togithub.com/BorysTheDev) in [dragonflydb/dragonfly#2902 - chore: add help string for memory_fiberstack_vms_bytes by [@​romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2903 - fix(sanitizers): false positive fail on multi_test::Eval by [@​kostasrim](https://togithub.com/kostasrim) in [dragonflydb/dragonfly#2896 - chore: pull helio and add ipv6 replication test by [@​dranikpg](https://togithub.com/dranikpg) in [dragonflydb/dragonfly#2889 - chore: add ipv6 support for native linux release by [@​romange](https://togithub.com/romange) in [dragonflydb/dragonfly#2908 ##### Huge thanks to all the contributors! ❤️ **Full Changelog**: dragonflydb/dragonfly@v1.16.0...v1.17.0 </details> <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zMDEuNSIsInVwZGF0ZWRJblZlciI6IjM3LjMwMS41IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJyZW5vdmF0ZS9jb250YWluZXIiLCJ0eXBlL21pbm9yIl19--> Co-authored-by: repo-jeeves[bot] <106431701+repo-jeeves[bot]@users.noreply.github.com>
A self-laundering iterator will enable us to, eventually, yield from fibers while holding an iterator. For example:
Why is this a good idea? Because it will enable yielding inside
PreUpdate()
which will allow breaking down of writing huge entries in small quantities to disk/network, eliminating the need to allocate huge chunks of memory just for serialization.Also, it'll probably unlock future developments as well, as yielding can be useful in other contexts.