Implement limbo resolution listen throttling #5310

dconeybe · 2020-04-08T02:29:38Z

This is a port of firebase/firebase-js-sdk#2790 and firebase/firebase-android-sdk#1374 to iOS.

The following description was copied from the first of those two PRs and pasted here for your convenience:

This change was motivated by the following bug report: firebase/firebase-js-sdk#2683. In summary, the issue was that when a large number of documents went into limbo (in this case 15,000 documents) then this would cause 15,000 document listens, which would result in "resource-exhausted" errors. The client would then back off for a bit but would try the listens again, which would again result in "resource-exhausted" errors. And the cycle would continue. Worst of all, the customer was being billed for all of these reads that had no beneficial effects for the clients.

In order to avoid this situation, this PR modifies the limbo resolution logic to "throttle" the limbo resolutions. It does this by allowing at most 100 concurrent limbo resolutions. Limbo resolutions in excess of this limit are queued up and not started until another limbo resolution completes. This fix should avoid the "resource-exhausted" errors and the infinite read loop.

…(it doesn't do anything yet though)

…he sync engine.

…ions"

wu-hui · 2020-04-08T16:11:09Z

Firestore/core/src/firebase/firestore/core/sync_engine.h

@@ -231,6 +240,16 @@ class SyncEngine : public remote::RemoteStoreCallback, public QueryEventSource {

  void TrackLimboChange(const LimboDocumentChange& limbo_change);

+  /**


Ultranit: I understand this is copied from other platforms. I'd still suggest to mention max_concurrent_limbo_resolutions_ in the comment.

wu-hui · 2020-04-08T16:27:04Z

Firestore/core/src/firebase/firestore/core/sync_engine.cc

-    limbo_targets_by_key_[key] = limbo_target_id;
+    active_limbo_resolutions_by_target_.emplace(limbo_target_id,
+                                                LimboResolution{key});
+    active_limbo_targets_by_key_[key] = limbo_target_id;


Is there special reason why [] is used here as opposed to emplace you used in the above line?

If not, we should be consistent here and use emplace in both?

So that particular line of code is not new but only changed due to the renaming of the variable limbo_targets_by_key_ to active_limbo_targets_by_key_. If I understand emplace() correctly, there is no benefit to using it over operator[] because limbo_target_id is just an int and therefore does not benefit from move semantics. I was also trying to minimize the unrelated changes so if you'd still prefer that I change it to emplace() then I will do that in a follow-up PR.

I see. I am OK with a separate PR to make it more consistent.

For what it's worth, emplace on a map is constructing the std::pair representing the key-value pair, so even though the limbo_target_id is trivially copyable, the pair is not because the key is a DocumentKey.

This is something of an academic concern though: we've structured our code so that vocabulary types like DocumentKey are cheaply copyable. They're immutable values that hide a std::shared_ptr, making a copy just a reference count bump. In this case emplace vs operator[] without moving both make a copy so there's essentially no difference (emplace with a moved key does save the copy, but that doesn't work here because the key is used below).

In any case, making these consistent is still worthwhile, and feel free to just inline that into this PR. A separate PR just for that small change is more effort than this change is worth.

Done. It turns out that this was the only place where [] needed to be changed to emplace.

wu-hui

Really just one place over the usage of [] and emplace. The other nit is optional.

…EnqueuedLimboResolutions(), as requested in code review.

dconeybe

Thanks for the review, @wu-hui!

dconeybe · 2020-04-08T17:06:12Z

Firestore/core/src/firebase/firestore/core/sync_engine.h

@@ -231,6 +240,16 @@ class SyncEngine : public remote::RemoteStoreCallback, public QueryEventSource {

  void TrackLimboChange(const LimboDocumentChange& limbo_change);

+  /**


dconeybe · 2020-04-08T18:41:51Z

Firestore/core/src/firebase/firestore/core/sync_engine.cc

-    limbo_targets_by_key_[key] = limbo_target_id;
+    active_limbo_resolutions_by_target_.emplace(limbo_target_id,
+                                                LimboResolution{key});
+    active_limbo_targets_by_key_[key] = limbo_target_id;


So that particular line of code is not new but only changed due to the renaming of the variable limbo_targets_by_key_ to active_limbo_targets_by_key_. If I understand emplace() correctly, there is no benefit to using it over operator[] because limbo_target_id is just an int and therefore does not benefit from move semantics. I was also trying to minimize the unrelated changes so if you'd still prefer that I change it to emplace() then I will do that in a follow-up PR.

wu-hui

LGTM

wilhuff · 2020-04-08T04:09:05Z

Firestore/core/src/firebase/firestore/core/sync_engine.h

@@ -90,7 +92,8 @@ class SyncEngine : public remote::RemoteStoreCallback, public QueryEventSource {
 public:
  SyncEngine(local::LocalStore* local_store,
             remote::RemoteStore* remote_store,
-             const auth::User& initial_user);
+             const auth::User& initial_user,
+             size_t maxConcurrentLimboResolutions);


snake case for variable names.

Oops. Fixed. Good catch.

wilhuff · 2020-04-08T19:36:28Z

Firestore/CHANGELOG.md

@@ -1,4 +1,6 @@
 # Unreleased
+- [changed] Firestore now limits the number of concurrent document lookups it
+  will perform when resolving inconsistencies in the local cache (#5310).


This really should link to the issue rather than the PR--it doesn't matter that the issue was in another repository. See below for a what a link looks like to an issue in firebase-js-sdk.

Done. Changed the PR number to the link to the issue firebase/firebase-js-sdk#449.

wilhuff · 2020-04-08T19:51:29Z

Firestore/core/src/firebase/firestore/core/sync_engine.cc

-    limbo_targets_by_key_[key] = limbo_target_id;
+    active_limbo_resolutions_by_target_.emplace(limbo_target_id,
+                                                LimboResolution{key});
+    active_limbo_targets_by_key_[key] = limbo_target_id;


For what it's worth, emplace on a map is constructing the std::pair representing the key-value pair, so even though the limbo_target_id is trivially copyable, the pair is not because the key is a DocumentKey.

This is something of an academic concern though: we've structured our code so that vocabulary types like DocumentKey are cheaply copyable. They're immutable values that hide a std::shared_ptr, making a copy just a reference count bump. In this case emplace vs operator[] without moving both make a copy so there's essentially no difference (emplace with a moved key does save the copy, but that doesn't work here because the key is used below).

In any case, making these consistent is still worthwhile, and feel free to just inline that into this PR. A separate PR just for that small change is more effort than this change is worth.

dconeybe · 2020-04-15T16:00:12Z

Note that the ASAN check failure is a known issue with the build system: #5316 (comment). I will ignore that failure and go ahead and merge.

dconeybe added 7 commits April 7, 2020 10:54

Add CHANGELOG.md entry so I don't forget this time.

8c2978b

Remove 'no-ios' tags from throttling spec tests in limbo_spec_test.json

7ae4024

Add maxConcurrentLimboResolutions constructor argument to SyncEngine …

34c32f4

…(it doesn't do anything yet though)

Read maxConcurrentLimboResolutions from config and pass it along to t…

20ad3d6

…he sync engine.

Implement throttling in SyncEngine

44847f3

Rename "CurrentLimboDocuments" methods to "ActiveLimboDocumentResolut…

125ede3

…ions"

Update spec tests to validate the enqueued limbo docs

04ab6fa

dconeybe added the api: firestore label Apr 8, 2020

dconeybe self-assigned this Apr 8, 2020

googlebot added the cla: yes label Apr 8, 2020

dconeybe added 2 commits April 7, 2020 22:34

Fix #include from <queue> to <set> in FSTSpecTests.mm

21bd18f

Update CHANGELOG.md with the PR number of this PR.

a515f85

dconeybe marked this pull request as ready for review April 8, 2020 13:33

dconeybe requested a review from wu-hui April 8, 2020 13:33

dconeybe assigned wu-hui and unassigned dconeybe Apr 8, 2020

wu-hui reviewed Apr 8, 2020

View reviewed changes

wu-hui requested changes Apr 8, 2020

View reviewed changes

wu-hui assigned dconeybe and unassigned wu-hui Apr 8, 2020

Add mention of max_concurrent_limbo_resolutions_ in the docs for Pump…

fad0b1e

…EnqueuedLimboResolutions(), as requested in code review.

dconeybe commented Apr 8, 2020

View reviewed changes

dconeybe assigned wu-hui and unassigned dconeybe Apr 8, 2020

wu-hui approved these changes Apr 8, 2020

View reviewed changes

wu-hui assigned dconeybe and unassigned wu-hui Apr 8, 2020

wilhuff reviewed Apr 8, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into LimboThrottling

2993a42

Applied code review feedback

ee16a9e

dconeybe merged commit cd6d0f1 into master Apr 15, 2020

dconeybe deleted the dconeybe/LimboThrottling branch April 15, 2020 16:01

ryanwilson pushed a commit that referenced this pull request Apr 24, 2020

Implement limbo resolution listen throttling (#5310)

de45766

firebase locked and limited conversation to collaborators May 16, 2020

		@@ -231,6 +240,16 @@ class SyncEngine : public remote::RemoteStoreCallback, public QueryEventSource {

		void TrackLimboChange(const LimboDocumentChange& limbo_change);

		/**

Implement limbo resolution listen throttling #5310

Implement limbo resolution listen throttling #5310

Uh oh!

Conversation

dconeybe commented Apr 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-hui left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dconeybe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-hui left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dconeybe commented Apr 15, 2020

Uh oh!

Uh oh!

wu-hui left a comment •

edited

Loading