C++ migration: port `FSTTransaction` #2362

var-const · 2019-02-08T00:22:53Z

No description provided.

var-const · 2019-02-08T23:54:21Z

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

+   */
+  util::Status last_write_error_;
+
+  std::unordered_map<model::DocumentKey,


FSTTransaction used a SortedMap, but it seems to me that unordered_map is more appropriate here: order does not matter, and the map isn't shared across different objects.

FSTTransaction used a SortedMap

Technically, it uses a std::map, right? (Just to make sure we're looking at the same thing). According to our git history, this used to be a NSMutableDictionary. So I suspect the conversion from that to std::map was slightly wrong, and it should've been unsorted_map instead.

Right... It's the Web SDK that uses a SortedMap, which I think I originally duplicated and then thought that SortedMap was there from the start. Android uses the standard library HashMap.

var-const · 2019-02-08T23:55:34Z

Firestore/core/src/firebase/firestore/core/transaction.mm

+
+absl::optional<SnapshotVersion> Transaction::GetVersion(
+    const DocumentKey& key) const {
+  auto found = read_versions_.find(key);


This method doesn't exist on other platforms, but makes the code accessing the map more similar (other platforms use some Get which returns an element or null).

var-const · 2019-02-08T23:56:43Z

Firestore/core/src/firebase/firestore/remote/remote_store.h

@@ -177,7 +178,9 @@ class RemoteStore : public TargetMetadataProvider,
  void AddToWritePipeline(FSTMutationBatch* batch);

  /** Returns a new transaction backed by this remote store. */
-  FSTTransaction* CreateTransaction();
+  // TODO(c++14): return a plain value when it becomes possible to move


Returning a shared_ptr is largely an optimization. The transaction is copied into a lambda more than once, and moving into a lamdba is not possible in C++11.

Copying shared pointers around is kinda slow too. Depending on it's size/complexity, you might be better off just copying the underlying object. (I wrote up a quick/naive benchmark and observed that for small objects, it was faster to just copy them. The cut-over point was ~4k for my (simple) test object, though I would imagine it would be smaller for a core::Transaction.)

It doesn't really matter here... any optimization we do will be dwarfed by the round trip times to the server that a transaction requires. Feel free to keep this as a shared_ptr or convert to plain.

Agreed that it's unlikely to be a big deal either way. A couple of arguments in favor of shared_ptr:

Transaction has a vector and an unordered_map as its members, which require dynamic memory allocation;

once we start using C++ Mutation, the most natural way to store it would be as vector<unique_ptr<Mutation>>, which would make the class uncopyable (of course it's possible to write a custom copy constructor, but using shared_ptr for the whole Transaction now simply avoids the issue).

Does each lambda has to retain an ownership of Transaction? I expect each lambda only use the passed in Transaction instead of retain an ownership of it. Whoever started a transaction would be the sole owner of the Transaction.

The problem is the lambda in FSTSyncEngine.mm. It creates a Transaction as a local variable and immediately passes it into a lambda.

var-const · 2019-02-09T00:01:06Z

Firestore/core/src/firebase/firestore/core/transaction.mm

+  absl::optional<SnapshotVersion> existing_version = GetVersion(doc.key);
+  if (existing_version.has_value()) {
+    if (doc_version != existing_version.value()) {
+      return Status{// This transaction will fail no matter what.


As discussed, this changes the behavior -- previously, iOS would unconditionally forbid reading the same document more than once. The new behavior is same as other platforms.

rsgowman · 2019-02-11T15:34:43Z

Firestore/core/src/firebase/firestore/remote/remote_store.h

@@ -177,7 +178,9 @@ class RemoteStore : public TargetMetadataProvider,
  void AddToWritePipeline(FSTMutationBatch* batch);

  /** Returns a new transaction backed by this remote store. */
-  FSTTransaction* CreateTransaction();
+  // TODO(c++14): return a plain value when it becomes possible to move


Copying shared pointers around is kinda slow too. Depending on it's size/complexity, you might be better off just copying the underlying object. (I wrote up a quick/naive benchmark and observed that for small objects, it was faster to just copy them. The cut-over point was ~4k for my (simple) test object, though I would imagine it would be smaller for a core::Transaction.)

It doesn't really matter here... any optimization we do will be dwarfed by the round trip times to the server that a transaction requires. Feel free to keep this as a shared_ptr or convert to plain.

rsgowman · 2019-02-11T15:55:44Z

Firestore/core/test/firebase/firestore/remote/datastore_test.mm

-  EXPECT_EQ(resulting_docs, nullptr);
-  EXPECT_NE(resulting_error, nullptr);
+  EXPECT_TRUE(resulting_docs.empty());
+  EXPECT_FALSE(resulting_status.ok());


This looks to be ported accurately, but we don't seem to be checking the error code here (other than ensuring it's not ok). Should we?

rsgowman · 2019-02-11T16:12:18Z

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

+  /**
+   * Every time a document is read, this should be called to record its version.
+   * If we read two different versions of the same document, this will return an
+   * error through its out parameter. When the transaction is committed, the


"... this will return an error through its out parameter. ..."

rsgowman · 2019-02-11T16:13:29Z

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

+  /**
+   * Returns the precondition for a document if the operation is an update,
+   * based on the provided UpdateOptions. Will return none precondition if an
+   * error occurred, in which case it sets the error parameter.


"... sets the error parameter. "

rsgowman · 2019-02-11T16:21:42Z

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

+
+  /**
+   * Returns the precondition for a document if the operation is an update,
+   * based on the provided UpdateOptions. Will return none precondition if an


'UpdateOptions' is styled like a class, but I don't think any such class exists. (It's certainly not passed as a parameter here.) I suspect that's obsolete?

FWIW, the js repo uses this phrase: "Returns the precondition for a document if the operation is an update." (Though in the error case, it throws, so we'd still want to document what happens when an error occurs.)

Yep, it looks obsolete. Done.

rsgowman · 2019-02-11T16:22:06Z

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

+
+  /**
+   * Returns the precondition for a document if the operation is an update,
+   * based on the provided UpdateOptions. Will return none precondition if an


'Will return none precondition...'

The implementation returns a failed status rather than a none precondition.

Done. Thanks!

rsgowman · 2019-02-11T16:31:59Z

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

+   */
+  util::Status last_write_error_;
+
+  std::unordered_map<model::DocumentKey,


FSTTransaction used a SortedMap

Technically, it uses a std::map, right? (Just to make sure we're looking at the same thing). According to our git history, this used to be a NSMutableDictionary. So I suspect the conversion from that to std::map was slightly wrong, and it should've been unsorted_map instead.

rsgowman · 2019-02-11T16:57:23Z

Firestore/Source/API/FIRTransaction.mm

+        }
+
+        HARD_ASSERT(documents.size() == 1, "Mismatch in docs returned from document lookup.");
+        FSTMaybeDocument *internalDoc = documents.back();


It clearly makes no difference, but OOC, why back instead of front?

I guess back seems to me as if it's more natural for a vector. Changed to front.

zxu123

LGTM. Just have some question on the ownership of Transaction.

zxu123 · 2019-02-11T20:33:16Z

Firestore/Source/API/FIRTransaction.mm


 NS_ASSUME_NONNULL_BEGIN

 #pragma mark - FIRTransaction

 @interface FIRTransaction ()

- (instancetype)initWithTransaction:(FSTTransaction *)transaction
+- (instancetype)initWithTransaction:(std::shared_ptr<Transaction>)transaction


Do we actually want to have shared ownership here? I expect the FIRTransaction will have sole ownership, maybe?

zxu123 · 2019-02-11T20:48:06Z

Firestore/core/src/firebase/firestore/remote/remote_store.h

@@ -177,7 +178,9 @@ class RemoteStore : public TargetMetadataProvider,
  void AddToWritePipeline(FSTMutationBatch* batch);

  /** Returns a new transaction backed by this remote store. */
-  FSTTransaction* CreateTransaction();
+  // TODO(c++14): return a plain value when it becomes possible to move


Does each lambda has to retain an ownership of Transaction? I expect each lambda only use the passed in Transaction instead of retain an ownership of it. Whoever started a transaction would be the sole owner of the Transaction.

* master: E2e tests split (#2401) Split auth api tests into multiple cases (#2399) Add a ToString function to create human-readable debug descriptions (#2384) Split Auth unit tests (#2396) Create umbrella header for FIAM (#2392) Deprecate setMinimumSessionInterval (#2305) Forbid queries endAt an uncommitted server timestamp. (#2382) update changelog for release (#2383) Fix FIAM Travic CI issues (#2380) Remove swift sample for Auth (#2371) C++ migration: port `FSTTransaction` (#2362) Fix a bug where unlinking emailpassword doesn’t remove provider data (#2370) Fix a bug where sign in with Game Center doesn’t return additional user info (#2368) Firebase In-app messaging callbacks (#2354) Fix a bug where sign in with email link always return isNewUser as false (#2363) C++ migration: eliminate `FSTRemoteStore` (#2338)

var-const added 30 commits January 15, 2019 17:15

Remove extra imports and includes

76b206b

Port

7dcb9ae

Forgotten method

88ba1a7

style.sh

895bf45

Forgotten the main part!

b66c5d2

Fixup unit tests

3f5fdae

Wip

5ce04b8

Step back

3dc3170

Core compiles

9d1f931

Merge branch 'master' into varconst/fst-watch-change

7ea798e

quick fix

84842be

Test helpers

e5132be

MockDatastore

6c85090

FSTLocalStoreTests

f7a90b5

wip

267963f

wip

60a5726

FSTRemoveEventTests

dd9d98d

FSTSerializerBetaTests

aec3f54

wip

b012929

wip

1da0672

wip

e0f77f2

wip

df5cd00

wip

5d3313a

wip

31e5f3d

Ship it!

0637243

use changes

26b39c7

quick fix

2cab650

Unit tests pass

ca88d8c

style.sh

2c0a7ef

Small fixes

52b131d

var-const added 11 commits February 8, 2019 14:53

Merge branch 'master' into varconst/fst-transaction3

edf51bd

undo

2e0a8c3

fix typo

15f7613

Minor style change

37b1d46

temp code

32bfb2a

Fix

3c9a898

Conformance

50f69ed

style.sh

02b1c80

remove accidental

7f47e36

Fix

9efdd6a

Fix

e88834e

var-const commented Feb 8, 2019

View reviewed changes

Fix

3f0ed3b

var-const commented Feb 9, 2019

View reviewed changes

var-const changed the title ~~[DO NOT MERGE] [NOT READY FOR REVIEW]~~ C++ migration: port FSTTransaction Feb 9, 2019

var-const requested review from rsgowman and zxu123 February 9, 2019 00:01

var-const assigned rsgowman and zxu123 Feb 9, 2019

var-const added the api: firestore label Feb 9, 2019

rsgowman approved these changes Feb 11, 2019

View reviewed changes

rsgowman assigned var-const and unassigned rsgowman Feb 11, 2019

Review feedback

89e4b14

zxu123 reviewed Feb 11, 2019

View reviewed changes

zxu123 removed their assignment Feb 11, 2019

var-const merged commit c422806 into master Feb 12, 2019

var-const deleted the varconst/fst-transaction3 branch February 12, 2019 00:14

firebase locked and limited conversation to collaborators Oct 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C++ migration: port `FSTTransaction` #2362

C++ migration: port `FSTTransaction` #2362

var-const commented Feb 8, 2019

var-const Feb 8, 2019

rsgowman Feb 11, 2019

var-const Feb 11, 2019

var-const Feb 8, 2019

var-const Feb 8, 2019

rsgowman Feb 11, 2019

var-const Feb 11, 2019

rsgowman Feb 11, 2019

zxu123 Feb 11, 2019

var-const Feb 11, 2019

var-const Feb 9, 2019

rsgowman Feb 11, 2019

rsgowman Feb 11, 2019

var-const Feb 11, 2019

rsgowman Feb 11, 2019

var-const Feb 11, 2019

rsgowman Feb 11, 2019

var-const Feb 11, 2019

rsgowman Feb 11, 2019

var-const Feb 11, 2019

rsgowman Feb 11, 2019

var-const Feb 11, 2019

rsgowman Feb 11, 2019

rsgowman Feb 11, 2019

var-const Feb 11, 2019

zxu123 left a comment

zxu123 Feb 11, 2019

zxu123 Feb 11, 2019

C++ migration: port FSTTransaction #2362

C++ migration: port FSTTransaction #2362

Conversation

var-const commented Feb 8, 2019

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

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

zxu123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

C++ migration: port `FSTTransaction` #2362

C++ migration: port `FSTTransaction` #2362