feat: yield when serialization is in progress #3220

kostasrim · 2024-06-26T07:04:58Z

No description provided.

kostasrim · 2024-06-26T07:06:06Z

src/server/snapshot.h

@@ -171,6 +171,8 @@ class SliceSnapshot {
    size_t savecb_calls = 0;
    size_t keys_total = 0;
  } stats_;
+
+  bool bucket_ser_in_progress_ = false;


I will remove this, it suffices to use the other bool above

kostasrim · 2024-06-26T07:06:23Z

src/server/snapshot.cc

+  }
+
+  // If a bucket is being serialized we need to block
+  // maybe worth adding it on an event queue?


We can notify and wait on a queue here, but is it really worth it?

What's the upside of using a queue instead of just blocking?
I don't understand your proposal, sorry

kostasrim · 2024-06-26T07:08:35Z

src/server/snapshot.cc

    SerializeEntry(db_index, it->first, it->second, nullopt, serializer_.get());
    ++it;
  }
+  og.SetVersion(snapshot_version_);


We should not bump the version before we yield because other fibers might check against the version and decide to move on instead of block and cause a bucket split

I'm not sure if this is interesting on its own, as:

other fibers might check against the version and decide to move on instead of block and cause a bucket split

If that other fiber is older, it would have been called by now (we call by the order of snapshot versions), and if it's newer it wouldn't matter if the value is snapshot_version_ or smaller (it can't be bigger). Furthermore, we shouldn't really get to the point in parallel in multiple fibers because we have the above (to-be-changed) polling.

Furthermore^2, if you set it to be after, we could actually serialize the same key twice in a rare case in which we call OnDbChange, block, but the Traverse() fiber continues and reaches this bucket. This will generate an invalid RDB, which Dragonfly will reject loading.

kostasrim · 2024-06-26T07:09:38Z

src/server/snapshot.cc

    return false;
+
+  while (bucket_ser_in_progress_) {


This will block all operations on the thread, even actions on different buckets. Effectively this means if the bucket is not serialized and there is serialization in progress. block

definitely do not actively poll. this will consume 100% CPU, causing alerts and such, especially on huge entries...

kostasrim · 2024-06-26T07:10:11Z

@chakaz draft so we can discuss :)

chakaz · 2024-06-26T09:37:00Z

src/server/snapshot.cc

+      DVLOG(3) << "Skipped " << it.segment_id() << ":" << it.bucket_id() << ":" << it.slot_id()
+               << " at " << v;
+      ++stats_.skipped;
+      return true;


My intuition is that check() should return true if the check passes (== all ok), i.e. the opposite of the logic, but that's a nit

chakaz · 2024-06-26T09:39:14Z

src/server/snapshot.cc

+  // Must be atomic because after we call it.snapshot_version_ we're starting
  // to send incremental updates instead of serializing the whole bucket: We must not
  // send the update until the initial SerializeBucket is called.
  // Relying on the atomicity of SerializeBucket is Ok here because only one thread may handle this
  // bucket.
  FiberAtomicGuard fg;


Isn't the whole point of this PR to remove the atomicity requirement?
I.e. instead of fixing the typo in the comment, you should remove it (as well as the FiberAtomicGuard)

I didn't remove it since we (yet) don't yield. We will yield when we actually start serializing in chunks but I am happy to remove this on this PR -- no objections

src/server/snapshot.cc

chakaz · 2024-06-26T10:05:33Z

src/server/snapshot.cc

    SerializeEntry(db_index, it->first, it->second, nullopt, serializer_.get());
    ++it;
  }
+  og.SetVersion(snapshot_version_);


I'm not sure if this is interesting on its own, as:

other fibers might check against the version and decide to move on instead of block and cause a bucket split

If that other fiber is older, it would have been called by now (we call by the order of snapshot versions), and if it's newer it wouldn't matter if the value is snapshot_version_ or smaller (it can't be bigger). Furthermore, we shouldn't really get to the point in parallel in multiple fibers because we have the above (to-be-changed) polling.

Furthermore^2, if you set it to be after, we could actually serialize the same key twice in a rare case in which we call OnDbChange, block, but the Traverse() fiber continues and reaches this bucket. This will generate an invalid RDB, which Dragonfly will reject loading.

chakaz · 2024-06-26T10:06:26Z

src/server/snapshot.cc

+  }
+
+  // If a bucket is being serialized we need to block
+  // maybe worth adding it on an event queue?


What's the upside of using a queue instead of just blocking?
I don't understand your proposal, sorry

chakaz · 2024-06-26T10:09:33Z

src/server/snapshot.cc

Note that in addition to snapshot.cc, we also have streamer.cc which has some very similar logic.
I would say that it's not worth modifying, as it's way more rarely used (only during slot migrations), but the thing is, it must support blocking as well, as the underlying value serialization will block :|

adiholden · 2024-06-26T12:40:11Z

src/server/snapshot.cc

  // to send incremental updates instead of serializing the whole bucket: We must not
  // send the update until the initial SerializeBucket is called.
  // Relying on the atomicity of SerializeBucket is Ok here because only one thread may handle this
  // bucket.
  FiberAtomicGuard fg;
  DCHECK_LT(it.GetVersion(), snapshot_version_);

+  bucket_ser_in_progress_ = true;


this boolean is the same as serialize_bucket_running_

yes yes, this was for testing, it's a draft PR I will clean this up -- I wanted to discuss it with Shahar

adiholden · 2024-06-26T12:45:22Z

src/server/snapshot.cc

+  // maybe worth adding it on an event queue?
+  while (bucket_ser_in_progress_) {
+    ThisFiber::Yield();
+  }


it could be that the bucket was already serialized by the time you run this was sleeping. i.e snapshot version for this bucket was update. you need to check it again after the sleep

nice catch!

chakaz · 2024-06-30T07:15:30Z

src/server/journal/streamer.cc

@@ -312,7 +310,7 @@ bool RestoreStreamer::WriteBucket(PrimeTable::bucket_iterator it) {
 void RestoreStreamer::OnDbChange(DbIndex db_index, const DbSlice::ChangeReq& req) {
  DCHECK_EQ(db_index, 0) << "Restore migration only allowed in cluster mode in db0";

-  FiberAtomicGuard fg;
+  { std::unique_lock<util::fb2::Mutex> lk(bucket_ser_mu_); }


why do you put it on its own scope? It will be freed immediately after, which will mean that other OnDbChange callbacks could run in parallel, no?

I don't think so this is the case. Once it acquires the lock, it will drop it immediately but it won't preempt. It will preempt potentially when we call WriteBucket. All the steps between dropping the lock on line 313 and calling WriteBucket are atomic, there is no preemption. However, when we call WriteBucket we will reqacquire the lock on the same fiber before we preempt. Think of this {} as transferring ownership of the lock without explicitly doing it (e.g. lk.unlock() before we call WriteBucket

Some thoughts on what you wrote:

Is what you wrote also true for CVCUponInsert()? I see that it uses a callback. I think it's true, but you should verify if we're counting on that

I know that "regular" mutexes (i.e. non fiber ones) do not guarantee that, even if the lock is unlocked, it will not preempt to other threads. I don't know if that's true here, but I guess it is. In other words, this may not guarantee a mutex transfer without yielding in between

I understand that WriteBucket() locks this lock, so I don't see any benefit to lock (and immediately release) here as well. Why even do that?

There might be a subtle bug here: CVCUponInsert() might call WriteBucket() multiple times (over multiple buckets). If we lose the mutex to some other operation, which touches that other bucket, it might change the underlying structure causing us to lose some changes

I'd recommend adding a WriteBucketNoLock() method which will be called here and in WriteBucket(). Bonus points if you add ABSL_LOCKS_EXCLUDED and ABSL_LOCKS_REQUIRED annotations to let the compiler verify we don't create deadlocks

I also dont see why we need to lock and release here, as if we lock in WriteBucket this should be enough.
@chakaz Regarding the comment that CVCUponInsert calls WriteBucket multiple times, I think that ok.. because what we care is that we dont break the bucket into multiple pieces but I dont think we should care if CVCUponInsert serializes one bucket we will preempt , serialize onother bucket from different flow and than go back to continue serialize the next buckets from CVCUponInsert. Is that right?

@chakaz

Yes we do not need the lock OnDbChange that was a mistake.

I know that "regular" mutexes (i.e. non fiber ones) do not guarantee that, even if the lock is unlocked, it will not preempt to other threads. I don't know if that's true here, but I guess it is. In other words, this may not guarantee a mutex transfer without yielding in between

Regular mutexes behave differently when notifying other threads to make progress. If this code was running on thread a but wakes up thread b and releases the mutex then it's a race for which thread locks the mutex. This is not true for fibers:

90 void Mutex::unlock() { 91 detail::FiberInterface* active = detail::FiberActive(); 92 93 unique_lock lk(wait_queue_splk_); 94 CHECK(owner_ == active); 95 owner_ = nullptr; 96 97 wait_queue_.NotifyOne(active); 98 }

And if you follow NotifyOne it calls NotifyImpl which calls ActivateOther

116 117 // Schedules another fiber without switching to it. 118 // other can belong to another thread. 119 void ActivateOther(FiberInterface* other); 120

The gist here is a) That ActivateOther adds the fiber to the ready queue b) that in our case all the interesting fibers (the fiber that runs OnDbChange && IterateFB) that use this mutex run on the same thread, this is both true for SaveStagesController && FullSyncFb for replication (on RDB version they do push on the same serialization channel which is fine since we don’t (yet until phase 2) split individual slots.

+1 for the rest

@adiholden won't happen, when we release the lock we continue running on the same fiber, we don't switch.

Plz let me know if I missed anything

@adiholden

because what we care is that we dont break the bucket into multiple pieces

We do want to break the bucket into multiple pieces, otherwise what's the point ? (edit: we just don't want to allow interleaved bucket serialization per SliceSnapshot)

adiholden · 2024-06-30T14:35:39Z

src/server/journal/streamer.cc

@@ -312,7 +310,7 @@ bool RestoreStreamer::WriteBucket(PrimeTable::bucket_iterator it) {
 void RestoreStreamer::OnDbChange(DbIndex db_index, const DbSlice::ChangeReq& req) {
  DCHECK_EQ(db_index, 0) << "Restore migration only allowed in cluster mode in db0";

-  FiberAtomicGuard fg;
+  { std::unique_lock<util::fb2::Mutex> lk(bucket_ser_mu_); }


I also dont see why we need to lock and release here, as if we lock in WriteBucket this should be enough.
@chakaz Regarding the comment that CVCUponInsert calls WriteBucket multiple times, I think that ok.. because what we care is that we dont break the bucket into multiple pieces but I dont think we should care if CVCUponInsert serializes one bucket we will preempt , serialize onother bucket from different flow and than go back to continue serialize the next buckets from CVCUponInsert. Is that right?

adiholden · 2024-06-30T14:39:23Z

src/server/snapshot.h

@@ -171,6 +172,8 @@ class SliceSnapshot {
    size_t savecb_calls = 0;
    size_t keys_total = 0;
  } stats_;
+
+  util::fb2::Mutex bucket_ser_mu_;


you have different mutex one for snapshot and one for cluster, I think this is a problem.
If we have a snapshot created at the same time there is a migration created and we yield in the bucket serialization

feat: yield when serialization is in progress

96c6aef

kostasrim self-assigned this Jun 26, 2024

kostasrim commented Jun 26, 2024

View reviewed changes

kostasrim requested a review from chakaz June 26, 2024 07:09

kostasrim mentioned this pull request Jun 26, 2024

Allow yielding in OnRecordChange and OnBucketSave #3222

Open

chakaz reviewed Jun 26, 2024

View reviewed changes

adiholden reviewed Jun 26, 2024

View reviewed changes

kostasrim added 2 commits June 28, 2024 13:03

replace bool with mutex

babfc79

update restore streamer

de9b143

kostasrim marked this pull request as ready for review June 28, 2024 10:20

kostasrim requested review from chakaz and adiholden June 30, 2024 06:57

chakaz reviewed Jun 30, 2024

View reviewed changes

kostasrim requested a review from chakaz June 30, 2024 07:44

adiholden requested changes Jun 30, 2024

View reviewed changes

adiholden reviewed Jun 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: yield when serialization is in progress #3220

feat: yield when serialization is in progress #3220

kostasrim commented Jun 26, 2024

kostasrim Jun 26, 2024

kostasrim Jun 26, 2024

chakaz Jun 26, 2024

kostasrim Jun 26, 2024

chakaz Jun 26, 2024

kostasrim Jun 26, 2024

chakaz Jun 26, 2024

kostasrim commented Jun 26, 2024

chakaz Jun 26, 2024

chakaz Jun 26, 2024

kostasrim Jun 26, 2024

chakaz Jun 26, 2024

chakaz Jun 26, 2024

chakaz Jun 26, 2024

adiholden Jun 26, 2024

kostasrim Jun 26, 2024

adiholden Jun 26, 2024

kostasrim Jun 26, 2024

chakaz Jun 30, 2024

kostasrim Jun 30, 2024

chakaz Jun 30, 2024

adiholden Jun 30, 2024

kostasrim Jun 30, 2024

kostasrim Jun 30, 2024 •

edited

Loading

adiholden Jun 30, 2024

adiholden Jun 30, 2024

feat: yield when serialization is in progress #3220

Are you sure you want to change the base?

feat: yield when serialization is in progress #3220

Conversation

kostasrim commented Jun 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim commented Jun 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim Jun 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim Jun 30, 2024 •

edited

Loading