From 450d7a18ffffbaeb8722b2d84ec181fbff7e91bb Mon Sep 17 00:00:00 2001 From: Konstantin Varlamov Date: Fri, 11 May 2018 21:43:25 -0400 Subject: Firestore C++: make FSTDispatchQueue delegate to C++ implementation (#1240) FSTDispatchQueue now doesn't contain any logic of its own and instead just passes through all method calls to AsyncQueue (backed by an ExecutorLibdispatch). --- .../src/firebase/firestore/util/async_queue.cc | 6 +- .../firebase/firestore/util/executor_libdispatch.h | 11 +-- .../firestore/util/executor_libdispatch.mm | 80 +++++++++------------- .../src/firebase/firestore/util/executor_std.h | 1 - 4 files changed, 40 insertions(+), 58 deletions(-) (limited to 'Firestore/core/src/firebase') diff --git a/Firestore/core/src/firebase/firestore/util/async_queue.cc b/Firestore/core/src/firebase/firestore/util/async_queue.cc index 71f5cc5..81aac7c 100644 --- a/Firestore/core/src/firebase/firestore/util/async_queue.cc +++ b/Firestore/core/src/firebase/firestore/util/async_queue.cc @@ -32,6 +32,8 @@ AsyncQueue::AsyncQueue(std::unique_ptr executor) is_operation_in_progress_ = false; } +// TODO(varconst): assert in destructor that the queue is empty. + void AsyncQueue::VerifyIsCurrentExecutor() const { FIREBASE_ASSERT_MESSAGE( executor_->IsCurrentExecutor(), @@ -97,8 +99,8 @@ void AsyncQueue::VerifySequentialOrder() const { // This is the inverse of `VerifyIsCurrentQueue`. FIREBASE_ASSERT_MESSAGE( !is_operation_in_progress_ || !executor_->IsCurrentExecutor(), - "Enforcing sequential order failed: currently executing operations " - "cannot enqueue more operations " + "Enqueue methods cannot be called when we are already running on " + "target executor" "(this queue's executor: '%s', current executor: '%s')", executor_->Name().c_str(), executor_->CurrentExecutorName().c_str()); } diff --git a/Firestore/core/src/firebase/firestore/util/executor_libdispatch.h b/Firestore/core/src/firebase/firestore/util/executor_libdispatch.h index 85c34f8..f913fe5 100644 --- a/Firestore/core/src/firebase/firestore/util/executor_libdispatch.h +++ b/Firestore/core/src/firebase/firestore/util/executor_libdispatch.h @@ -45,10 +45,10 @@ namespace internal { // Generic wrapper over `dispatch_async_f`, providing `dispatch_async`-like // interface: accepts an arbitrary invocable object in place of an Objective-C // block. -void DispatchAsync(const dispatch_queue_t queue, std::function&& work); +void DispatchAsync(dispatch_queue_t queue, std::function&& work); // Similar to `DispatchAsync` but wraps `dispatch_sync_f`. -void DispatchSync(const dispatch_queue_t queue, std::function work); +void DispatchSync(dispatch_queue_t queue, std::function work); class TimeSlot; @@ -56,9 +56,7 @@ class TimeSlot; // a dedicated serial dispatch queue. class ExecutorLibdispatch : public Executor { public: - ExecutorLibdispatch(); explicit ExecutorLibdispatch(dispatch_queue_t dispatch_queue); - ~ExecutorLibdispatch(); bool IsCurrentExecutor() const override; std::string CurrentExecutorName() const override; @@ -79,11 +77,6 @@ class ExecutorLibdispatch : public Executor { } private: - // GetLabel functions are guaranteed to never return a "null" string_view - // (i.e. data() != nullptr). - absl::string_view GetCurrentQueueLabel() const; - absl::string_view GetTargetQueueLabel() const; - dispatch_queue_t dispatch_queue_; // Stores non-owned pointers to `TimeSlot`s. // Invariant: if a `TimeSlot` is in `schedule_`, it's a valid pointer. diff --git a/Firestore/core/src/firebase/firestore/util/executor_libdispatch.mm b/Firestore/core/src/firebase/firestore/util/executor_libdispatch.mm index 5491fec..597d450 100644 --- a/Firestore/core/src/firebase/firestore/util/executor_libdispatch.mm +++ b/Firestore/core/src/firebase/firestore/util/executor_libdispatch.mm @@ -28,13 +28,16 @@ absl::string_view StringViewFromDispatchLabel(const char* const label) { return label ? absl::string_view{label} : absl::string_view{""}; } -void RunSynchronized(const ExecutorLibdispatch* const executor, - std::function&& work) { - if (executor->IsCurrentExecutor()) { - work(); - } else { - DispatchSync(executor->dispatch_queue(), std::move(work)); - } +// GetLabel functions are guaranteed to never return a "null" string_view +// (i.e. data() != nullptr). +absl::string_view GetQueueLabel(const dispatch_queue_t queue) { + return StringViewFromDispatchLabel(dispatch_queue_get_label(queue)); +} +absl::string_view GetCurrentQueueLabel() { + // Note: dispatch_queue_get_label may return nullptr if the queue wasn't + // initialized with a label. + return StringViewFromDispatchLabel( + dispatch_queue_get_label(DISPATCH_CURRENT_QUEUE_LABEL)); } } // namespace @@ -51,15 +54,32 @@ void DispatchAsync(const dispatch_queue_t queue, std::function&& work) { }); } -void DispatchSync(const dispatch_queue_t queue, Executor::Operation work) { +void DispatchSync(const dispatch_queue_t queue, std::function work) { + FIREBASE_ASSERT_MESSAGE( + GetCurrentQueueLabel() != GetQueueLabel(queue), + "Calling DispatchSync on the current queue will lead to a deadlock."); + // Unlike dispatch_async_f, dispatch_sync_f blocks until the work passed to it // is done, so passing a reference to a local variable is okay. dispatch_sync_f(queue, &work, [](void* const raw_work) { - const auto unwrap = static_cast(raw_work); + const auto unwrap = static_cast*>(raw_work); (*unwrap)(); }); } +namespace { + +template +void RunSynchronized(const ExecutorLibdispatch* const executor, Work&& work) { + if (executor->IsCurrentExecutor()) { + work(); + } else { + DispatchSync(executor->dispatch_queue(), std::forward(work)); + } +} + +} // namespace + // Represents a "busy" time slot on the schedule. // // Since libdispatch doesn't provide a way to cancel a scheduled operation, once @@ -170,32 +190,15 @@ void TimeSlot::RemoveFromSchedule() { ExecutorLibdispatch::ExecutorLibdispatch(const dispatch_queue_t dispatch_queue) : dispatch_queue_{dispatch_queue} { } -ExecutorLibdispatch::ExecutorLibdispatch() - : ExecutorLibdispatch{dispatch_queue_create("com.google.firebase.firestore", - DISPATCH_QUEUE_SERIAL)} { -} - -ExecutorLibdispatch::~ExecutorLibdispatch() { - // Turn any operations that might still be in the queue into no-ops, lest - // they try to access `ExecutorLibdispatch` after it gets destroyed. Because - // the queue is serial, by the time libdispatch gets to the newly-enqueued - // work, the pending operations that might have been in progress would have - // already finished. - RunSynchronized(this, [this] { - for (auto slot : schedule_) { - slot->MarkDone(); - } - }); -} bool ExecutorLibdispatch::IsCurrentExecutor() const { - return GetCurrentQueueLabel().data() == GetTargetQueueLabel().data(); + return GetCurrentQueueLabel() == GetQueueLabel(dispatch_queue()); } std::string ExecutorLibdispatch::CurrentExecutorName() const { return GetCurrentQueueLabel().data(); } std::string ExecutorLibdispatch::Name() const { - return GetTargetQueueLabel().data(); + return GetQueueLabel(dispatch_queue()).data(); } void ExecutorLibdispatch::Execute(Operation&& operation) { @@ -243,29 +246,14 @@ void ExecutorLibdispatch::RemoveFromSchedule(const TimeSlot* const to_remove) { }); } -// GetLabel functions are guaranteed to never return a "null" string_view -// (i.e. data() != nullptr). -absl::string_view ExecutorLibdispatch::GetCurrentQueueLabel() const { - // Note: dispatch_queue_get_label may return nullptr if the queue wasn't - // initialized with a label. - return StringViewFromDispatchLabel( - dispatch_queue_get_label(DISPATCH_CURRENT_QUEUE_LABEL)); -} - -absl::string_view ExecutorLibdispatch::GetTargetQueueLabel() const { - return StringViewFromDispatchLabel( - dispatch_queue_get_label(dispatch_queue())); -} - // Test-only methods bool ExecutorLibdispatch::IsScheduled(const Tag tag) const { bool result = false; RunSynchronized(this, [this, tag, &result] { - result = std::find_if(schedule_.begin(), schedule_.end(), - [&tag](const TimeSlot* const operation) { - return *operation == tag; - }) != schedule_.end(); + result = std::any_of( + schedule_.begin(), schedule_.end(), + [&tag](const TimeSlot* const operation) { return *operation == tag; }); }); return result; } diff --git a/Firestore/core/src/firebase/firestore/util/executor_std.h b/Firestore/core/src/firebase/firestore/util/executor_std.h index 58ef96f..8f2be02 100644 --- a/Firestore/core/src/firebase/firestore/util/executor_std.h +++ b/Firestore/core/src/firebase/firestore/util/executor_std.h @@ -220,7 +220,6 @@ class ExecutorStd : public Executor { bool IsScheduled(Tag tag) const override; absl::optional PopFromSchedule() override; - private: using TimePoint = async::Schedule::TimePoint; // To allow canceling operations, each scheduled operation is assigned // a monotonically increasing identifier. -- cgit v1.2.3